Quickly addressing some points... loadClass() is only for dynamically-generated class names. require_once is used in all other cases.
No need for an interface if it doesn't solve a problem, right? I believe the framework has standardized on iconv. -Matt On Mon, Nov 10, 2008 at 1:42 PM, till <[EMAIL PROTECTED]> wrote: > 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 >
