-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hey Till,
> 1) In Zend_Text_Table::__construct, the first else is not needed > (throw is good enough). Alright. > 2) I'd refactor the if/elseif/else for $drawSeperator (in > Zend_Text_Table::render()) into its own function. I'd also refactor > some of those large if/else's. Hm no, the drawSeparator part uses and modifies too many local variables. > 3) Fix documentation in Zend_Text_Table::appendRow() > (InvalidArgumentException) What's wrong with the documentation? > 4) No idea what the convention is, but what about using > Zend_Loader::loadClass() instead of require_once? I don't care so much > about the exceptions, but the sub classes for example could be > Zend_Loader'ed. The convention currently tells to use require_once. > 5) Is it possible that Zend_Text_Table_Row and Zend_Text_Table_Column > share some code or maybe implement the same interface? Not really. > 6) You throw InvalidArgumentException in Zend_Text_Table_Row/_Column, > this should probably be Zend_Text_Table_InvalidArgumentException, or > something? That is an SPL-Exception, don't know if that is disallowed in ZF, Matthew? > 7) svn:keywords property missing in all files ;) I will fix that. > 8) In Zend_Table_Text_Column::setContent() you always cast to UTF-8, > while I know that UTF-8 is a standard (somewhat), I'd make it the > default and otherwise a configuration option for the developer. Also, > it would be nice if the code was able to determine the encoding (= > convenience) without relying on the developer to provide the encoding. It does only cast to utf-8 when you specify another charset. And yet it is convention over configuration (utf-8 expected, but any other charset allowed). > 9) In Zend_Table_Text_Column::setAlign() - make that array static, or > it's re-evaluated with each in_array(). Hm, yeah makes probably sense > 10) Is there a convention to use "or" or "||" ? I'd use one style only. That coems from a discussion with Thomas, I will return to ||. > 11) You use iconv and Matthew told me it's somewhat the standard. But > mbstring support would be nice as well. Might be only my personal > preference, but - to pull the performance card - I also heard it's > faster. :-) Iconv is standard, while mbstring isn't. > Just some feedback, I don't know about all the conventions yet. But > I'm trying to wrap my head around them. ;-) Thank you for your feedback! > Cheers, > Till ................................... : ___ _ ___ ___ ___ _ ___ : : | \ /_\ / __| _ \ _ (_) \ : : | |) / _ \\__ \ _/ / | |) | : : |___/_/:\_\___/_| |_|_\_|___/ : :........:........................: : Web : http://www.dasprids.de : : E-mail : [EMAIL PROTECTED] : : Jabber : [EMAIL PROTECTED] : : ICQ : 105677955 : :........:........................: -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkkYktoACgkQ0HfT5Ws789D8AQCgykF+TekaKylCFo0VpJoTOC01 cfMAoJiF8Dj9piSBX2aO2A4sFO4LN7BD =QG3R -----END PGP SIGNATURE-----
