Till Klampaeckel [2010-07-31 22:56 UTC] Hey John,
you'll obviously need to work on a few things in order to be accepted into PEAR, and I'd really welcome it if you do. :-)
Most, if not all issues can be identified by running PHP_CodeSniffer against your code:
http://pear.php.net/package/PHP_CodeSniffer
pear install PHP_CodeSniffer
phpcs /path/to/your/code/*
Then - this is more an added bonus, but not less important: tests. :-)
Let me know if you have any questions!
Till
Michael Gauthier [2010-08-26 13:41 UTC] For starters, you will need to reorganize your files to fit the PEAR conventions.
You package file structure should look something like:
/
- File/
- PDFReader/
- Base.php
- Decoder.php
- Form.php
- FormField.php
- Object.php
- Page.php
- PDFReader.php
- tests/
- examples/
- FormExample.php
- TextExample.php
- doc/
- README.txt
- ChangeLog.txt
- package.xml
You should pick a package name that follows the PEAR conventions. I suggest File_PDFReader as I used in my example directory structure.
Michael Gauthier [2010-08-26 13:43 UTC] Hmm... I indented my file list example, I swear! PEPr seems to have mangled it.
Try this version:
http://labs.silverorange.com/files/pear-pepr/File_PDFReader.txt
Michael Gauthier [2010-08-26 14:03 UTC] 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).
Till Klampaeckel [2011-01-06 12:11 UTC] Hey John,
please let us know if you have any questions on the things Mike and I mentioned. :-)
Especially the CS related items are hard to adhere to in the end. I suggest you check them out earlier.
Till
Michael Gauthier [2011-02-04 15:51 UTC] 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.
John Stokes [2011-02-04 18:21 UTC] Michael,
Basically, I'm in process on the the pass by reference and replacement arrays (#4, 10, & 11), and everything else has been addressed.
Thanks.
-John
|
