Chuck Burgess [2008-05-21 18:11 UTC] Nice compact API, and no external dependencies that I can see, so a light package :)
I'd probably change eof() and damaged() to isEof() and isDamaged()... I tend to use this convention for readability of such boolean checker methods.
You'll probably also want to look at returning exceptions rather than "false" on failures. This way it will fit itself into the PEAR architecture. You'll want to write your own JpegMarkerReaderException class that extends PEAR_Exception, even if you don't add anything new to your child exception. Probably though, you'll want to customize new child exceptions from your parent exception, to highlight more specific exception reasons (e.g. an "unable to open file" exception).
Last thing would be that if there's a published spec online somewhere that you based this on, include that link as an @link or @see tag in the class docblock. That can help with future maintenance, if/when your package attracts new developers to help with it.
Chuck Burgess [2008-05-21 18:27 UTC] One thing I missed... your class should be named Image_JpegMarkerReader, to fit in PEAR's namespacing convention.
Philippe Jausions [2008-05-21 20:13 UTC] - Package name (and class) should be Image_JpegMarkerReader or Image_JPEG_MarkerReader.
- As Chuck noted, throw exceptions on failures (your package's exception class should extend PEAR_Exception); for instance on corrupted files. FALSE is appropriate if this is not an error (i.e. it could be expected to not find something.)
- Look into implementing SPL's Iterator (or IteratorAggregate) interfaces, so it would be very easy to do a foreach() on all the markers.
- Run PHP_CodeSniffer on your class to help correct PEAR coding standard issues (4-space indent, all private members name should start with underscore "_", etc...)
- The "filename" member is not declared
- "$this->in" is atypical for a file pointer name. Not a deal breaker by any measure, since it's private, but $this->_fp or $this->_file are usually more common.
- You may consider returning NULL instead of FALSE for the standalone markers in skipMarkersIfNot()
Ken Guest [2008-05-22 08:21 UTC] This wouldn't be a vote breaker (although it is important):
I ran the phpcodesniffer, phpcs, on JpegMarkerReader.php using the PEAR coding standards as active - it reported some 256 errors and 1 warnings.
These are mostly indents and "Perl-style comments are not allowed" errors - fairly trivial to clean up.
On the brighter side, and there's always a brighter side, it's superb that you have documentation for the package already!
|
