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). >> 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? >> 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 >> 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. >> 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. >> 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. >> 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. 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(). Cheers, Till
