Re: [PEPr] Changes in proposal for File Formats::PDF Reader

Hi, Till. Yes I did, thanks. You can see some of my responses in the change log. But, in a nutshell: 1.) Till's advice about running phpcs is important. JS: I've brought my code into CodeSniffer compliance and generally re-run CodeSniffer before uploading each new version 2.) should the base class be marked as abstract? JS: The subclasses use the base class's routines directly rather than extending them, so abstract isn't appropriate in this case 3.) echo + exit should not be used in PEAR code. You should throw an exception instead. JS: I've removed all the echo/exit and die statements and replaced them with Exceptions. 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. JS: The str_replace and/or preg_replace calls have to be done in a specific order because PDF data structures are very fluid and can be nested arbitrarily. I haven't been able to get recursive RegExs to work, so that's the solution for now. Lookup arrays won't help in this case. 5.) Your debug output always uses HTML formatting. People might be using this package in a non-web context. JS: I include both command-line "n" and HTML "" so it can be used in either context. I keep the HTML code minimal so command line users will have very little HTML to wade through. 6.) Create exceptions specific to your package instead of throwing generic exceptions. This will allow developers to handle errors specific to your package. JS: v0.1.4 implemented package-specific exceptions. 7.) The die() function is not allowed to be used in PEAR. Throw an exception instead. JS: See #3. 8.) check for gzip before executing it. On Windows it likely won't work, for example. JS: I've added a test for gzip. The decoder now throws an Exception if it can't find any valid unzip function. 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. JS: I've spent a fair amount of time experimenting with this. I don't want to use mbstring since it isn't enabled by default. iconv seemed like a good solution for Unicode conversions at first, but I can't get it to reliably convert characters above ord(128). Also, that is only useful for character sets iconv supports, which don't include WinAnsi, MacExpert, or PDFdoc as far as I can tell. 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 JS: Objects are always passed by reference in PHP 5. Adding the & in front of a passed object makes no difference, except that it reminds the programmer (me) that s/he is working directly with the object. I pass by reference in two other cases: 1) read-only context, which helps save memory resources and 2) when I'm intentionally working directly on the referenced array/object/var. All other instances should be passed by value (if they aren't I goofed :