Philippe Jausions [2008-01-30 03:04 UTC] Quick review of Mogile.phps
- Check all your docblocks, some of the @access are not reflective of the declaration (also check order of tags, see PEAR CS docblock example)
- The open { for a method declaration is on the following line not on the same as the "function" keyword
- Run your code with E_ALL, things like "list($ip, $port) = split(':', $host, 2)" will spit a notice if $host doesn't contain a ":".
- BTW, use explode(':', $host, 2) instead of split()
- Why use microseconds as timeouts if they're going to be divided by 1000000?
- Pick a single quotation style throughout your package, preferably single quotes, and stick to it.
- Avoid things like "{$domain}class{$j}" when you could use $domain.'class'.$j or "Unable to open \"$data\"" vs. 'Unable to open"'.$data.'"' (more syntax coloring friendly)
- Add "b" when opening files (i.e. fopen($path, 'rb') vs. fopen($path, 'r')
- The code would be a little bit cleaner if instead of (this is just one example):
if ($words[0] == 'OK') {
parse_str(trim($words[1]), $response);
} else {
throw new File_Mogile_Exception('mogilefs: ' . trim($line));
}
you had:
if ($words[0] != 'OK') {
throw new File_Mogile_Exception('mogilefs: ' . trim($line));
}
parse_str(trim($words[1]), $response);
- In _store(), is !preg_match('/^http:\/\/([a-z0-9.-]*):([0-9]*)\/(.*)$/' really correct? That would match http://...:00/ (btw use ! as regex delimiter so you can use / without the need of escaping it.)
- I don't think the require_once 'PEAR.php'; is needed for the exceptions
Philippe Jausions [2008-01-30 03:12 UTC] A stream wrapper would be nice too :-D
Joe Stump [2008-01-30 03:43 UTC] - Regarding Philippe's statement "The open { for a method declaration is on the following line not on the same as the "function" keyword", the documentation clearly states to use K&R, which means putting the { on it's own line (In other words, ignore what Philippe said about that as what he said goes against the CS).
http://pear.php.net/manual/en/standards.funcdef.php
- Maybe instead of using a regular expression use Validate::uri()?
- I agree a stream wrapper would be awesome.
Chuck Burgess [2008-01-31 01:38 UTC] Check your license tags... they link to BSD, but docblock text says PHP license.
If PEAR.php does indeed end up being needed by the exception (as the code comment says), maybe the exception class file would be a better place to put the require_once('PEAR.php') line.
Maybe rename private function request() to _request(), to follow the naming convention of your other private function _store(). That probably means I should also hint at renaming your private vars $_socket and $_domain. Actually, why are the two timeout vars public?
Sounds like we're giving you lots of nitpick-the-details comments, which probably means the overall package idea is good, its API looks fine, and we reviewers are left to just be human Codesniffers :)
Steve Williams [2008-02-01 01:04 UTC] Thanks for your feedback. I've updated the package per your comments except as noted below.
- I'll add the stream wrapper to the wishlist.
- Chuck Burgess"Check your license tags... they link to BSD, but docblock text says PHP license." I didn't see "PHP" in the license tag, but I was missing the license name. I added "BSD" as the license name.
- Removed @access, as phpdoc picks it up from the code. I'll put it back in if you all prefer it.
- Philippe Jausions: "Why use microseconds as timeouts if they're going to be divided by 1000000?" There are two timeouts, one passed to a function that takes float seconds and the other to a function that takes separate seconds and microseconds args. I want to be more consistent than that, so I chose microseconds. If I change it to seconds, then somebody would be right to ask, "Why seconds, if you're just going to multiply by 100000?"
Chuck Burgess [2008-02-01 01:52 UTC] I see the change to the LICENSE text in Mogile's file-level docblock to show BSD, matching its @license tag, but the Exception file is still showing "3.0 of the PHP license" paired with the @license tag showing BSD. I'm guessing you want to change that LICENSE text to BSD also.
I keep bringing up the license clarifications only because I've seen how hard it can be to change/correct them once a package gets released.
Steve Williams [2008-02-01 02:13 UTC] Chuck Burgess: "the Exception file is still showing '3.0 of the PHP license' ..."
Doh! Thanks for the clarification. I totally missed that, as I was focused on the @license tag.
Fixed.
Steve Williams [2008-02-01 23:40 UTC] A few more changes:
- Changed timeout configs from microseconds to seconds because:
- Added File_Mogile::$curlTimeout, as suggested by Dormando on the MogileFS email list. (Since CURL_TIMEOUT is in seconds, this made two of three timeout configs that are natively seconds, so I made them all seconds for consistency.)
- Corrected @return for listKeys() method.
- Switched to curl_setopt_array() from multiple curl_setopt() calls.
Joe Stump [2008-02-01 23:46 UTC] "- Added File_Mogile::$curlTimeout, as suggested by Dormando on the MogileFS email list. (Since CURL_TIMEOUT is in seconds, this made two of three timeout configs that are natively seconds, so I made them all seconds for consistency.)"
I'd make that something more generic as you may some day wish to switch from curl to some other library (HTTP_Request, HttpRequest, etc.).
Steve Williams [2008-02-01 23:46 UTC] Right now, File_Mogile is configured through static class properties. Example:
File_Mogile::$curlTimeout = 4;
I considered switching to a setopt() method or individual methods like setCurlTimeout().
But two of the timeout configs are used only when the File_Mogile object is instantiated--which is when the connection to the MogileFS tracker is made. So this wouldn't work:
$mogile = new File_Mogile(array('example.com:7201', 'mydomain');
$mogile->setSocketTimeout(0.01);
I could fix that by deferring the connection until the first command is issued, but that's more complex and would make documenting the set...() methods more complex.
If more experienced PEAR folks have a strong preference for set...() methods, I'll change it. But my inclination is to leave the configs in the static properties.
Steve Williams [2008-02-02 01:09 UTC] Joe Stump: "I'd make (File_Mogile::$curlTimeout) something more generic ..."
Good point. I changed it to File_Mogile::$commandTimeout.
|
