-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 > I am just citing "open" parts. ;-) > > On Mon, Nov 10, 2008 at 9:00 PM, Ben Scholzen 'DASPRiD' > <[EMAIL PROTECTED]> >> 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. > > I can live with that, even though I don't understand it. ;-) It would > be cleaner (IMHO, of course).
You may always supply a patch ;) >>> 3) Fix documentation in Zend_Text_Table::appendRow() >>> (InvalidArgumentException) >> What's wrong with the documentation? > > So all of the sudden one of the components throws another exception? I exchanged them all, yes ^^ >>> 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. > > I couldn't find that. Let's change it. :D :P >>> 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. > > They could both implement "render()" from an interface and then add > whatever is necessary. Does make absolutely no sense. Also the signatures of them differ. There is simply no point in both sharing an interface. >>> 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? > > See above. As I said, all fixed. >>> 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). > > I know, but what if my output is different? I know plenty of devices > where the default encoding is not (yet) UTF-8. Making it configurable > would add flexibility for the developer. If you output is different, you can still convert to something else. But the default decorator uses UTF-8 characters anyway, and when you use the ASCII-Decorator, well then, the content should be ASCII as well, right? ;) >>> 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. mbstring is no standard module, while iconv is. So everybody has iconv, but not everybody has mbstring. > I don't know about that. mbstring is available for many, I am not sure > if for example it justifies wrapping the abstracting into a > Zend_Text_Encoding (or similar) component. > > Also, Zend_Text_Table_Row has no __construct(). And? > 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 iEYEARECAAYFAkkYrncACgkQ0HfT5Ws789AVpACeKSTVQ0kVkNvRKGTmwO4uX8/M jUkAmwc1B7OWBAZaMquW4vsTLO1Aw8H6 =kp+Z -----END PGP SIGNATURE-----
