Re: [PEPr] Comment on File Formats::PDF Reader

Hey, Michael, I did email back on your tips, though I noticed my email got truncated. Basically, I'm in process on the the pass by reference and replacement arrays, and everything else has been addressed. Here's a quick summary: Restructure directories. Done. 1.) Till's advice about running phpcs is important. Done. 2.) should the base class be marked as abstract? No. The subclasses use rather than extend the base class. 3.) echo + exit should not be used in PEAR code. You should throw an exception instead. Fixed. 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. In progress 5.) Your debug output always uses HTML formatting. People might be using this package in a non-web context. There was a semi-lengthy discussion about this on the list. I'm keeping the HTML formatting. 6.) Create exceptions specific to your package instead of throwing generic exceptions. This will allow developers to handle errors specific to your package. Done. 7.) The die() function is not allowed to be used in PEAR. Throw an exception instead. Fixed. 8.) check for gzip before executing it. On Windows it likely won't work, for example. Done. 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. Tried. Didn't work, so I backed out. 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 See below. 11.) Additionally, arrays in PHP5 use a copy-on-write technique that means you usually shouldn't worry about passing arrays by reference either. Also resulted in a discussion on the list. I misunderstood the mechanics of copy-on-write, so I'm (carefully) removing some of these. 12.) Catching an exception and then throwing it again and not doing anything else is pointless. (in PDFreader). Fixed. -John On Fri, 4 Feb 2011 15:51:26 +0000 (GMT), "Michael Gauthier" <mike@silverorange.com> wrote:

John, It looks like you've addressed many of the points I outlined, but several are still outstanding? Are you planing to work on those as well? Also, congrats on the new pre-release with UTF-8 encoding.