[PEPr] Comment on File Formats::PDF Reader
| From: | Michael Gauthier | Date: | Thu, 26 Aug 2010 14:03:35 +0000 |
| Subject: | [PEPr] Comment on File Formats::PDF Reader | ||
| References: | 1 | Groups: | php.pear.dev |
| Request: | Send a blank email to pear-dev+get-53730@lists.php.net to get a copy of this message | ||
Now for general feedback. This package does provide a unique feature not
provided by other PEAR packages. I'd like to see it become part of PEAR.
1.) Till's advice about running phpcs is important.
2.) should the base class be marked as abstract?
3.) echo + exit should not be used in PEAR code. You should throw an
exception instead.
4.) Use of search and replacement arrays might make some of the code
cleaner. I notice there are a few sections where you have half a dozen
str_replace calls in a row.
5.) Your debug output always uses HTML formatting. People might be using
this package in a non-web context.
6.) Create exceptions specific to your package instead of throwing generic
exceptions. This will allow developers to handle errors specific to your
package.
7.) The die() function is not allowed to be used in PEAR. Throw an
exception instead.
8.) check for gzip before executing it. On Windows it likely won't work,
for example.
9.) Can any of the character encoding stuff be made easier using mbstring
or iconv extensions? If so, I'd suggest using these packages as they're
likely faster and more stable.
10.) Passing parameters by reference can be dangerous and is often not
needed. For example, &$PDFdecoder passed to the PDFform constructor. In
PHP5, objects are not copied by default. See
http://www.php.net/manual/en/language.oop5.references.php
11.) Additionally, arrays in PHP5 use a copy-on-write technique that means
you usually shouldn't worry about passing arrays by reference either.
12.) Catching an exception and then throwing it again and not doing
anything else is pointless. (in PDFreader).
--
http://pear.php.net/pepr/pepr-proposal-show.php?id=641