-- till <[EMAIL PROTECTED]> wrote (on Monday, 10 November 2008, 10:42 PM +0100): > 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 > >> 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
It's not well-documented, but it *is* a requirement right now: use require_once when the name of the class is known ahead of time, and not generated dynamically. This convention makes it trivial to comment out the require_once statements, which means that we can then get a performance boost from autoloading. If we were to leave in the loadClass() calls, then loadClass() would always be run, producing more overhead than if we simply used the class and relied on autoloading (which, if an opcode cache is in place, might mean no work is done). > >> 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. But what's the point? An interface just for the sake of one? Or can you specify a good, architectural reason to do so? (I'm truly curious, not trying to attack here.) > >> 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. All exception classes in ZF should derive from Zend_Exception. Right now, that doesn't mean much, but in the future it may (if we introduce translatable error exceptions, or an exception handler, or ...). > > > 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. I have to agree with Till here. > > > 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. We encourage using iconv, as it's available in default PHP installs, while mbstring is not. If there's a specific feature of mbstring that the component could benefit from, it's allowed, but it would need to be optional functionality that can fall back to iconv. > Also, Zend_Text_Table_Row has no __construct(). Classes don't *need* constructors. :) -- Matthew Weier O'Phinney Software Architect | [EMAIL PROTECTED] Zend Framework | http://framework.zend.com/
