[TYPO3-team-core-v5] FLOW3: First code sprint of F3PR

[Robert Lemke]

Hi Christopher,

Am 16.09.2008 um 14:59 schrieb Christopher Hlubek:

>> 3) More explicit conditions.
>>
>> Somewhere you had this check:
>>
>>  if ($packageMetaXML->parties) {
>>
>> That's error prone and not explicit enough. Better:
>>
>>  if (key_exists('parties', $packageMetaXML->parties) &&
>> count($packageMetaXML->parties) > 0) {
>>
>
> Why is the "key_exists('parties', $packageMetaXML->parties)" and test
> for count necessary? Can you give a test case where the if fails?

In fact key_exists() is even worse - sorry. Somehow I kept using
that not-anymore-documented PHP function.

Anyway: I prefer strong comparisons like if ($something === TRUE) over
if ($something), at least if it's not immediately visible where the
value comes from and if you can be sure that it's really of the expected
type.

So even if "if ($parties)" works fine, I find "if (isset($parties) &&  
count($parties) > 0)
more readable.

>> 4) Nit-picking on method names.
>>
>>       /**
>>        * Return package meta as XML string
>>        *
>>        * @return string The package meta in XML
>>        * @author Christopher Hlubek <hlubek at networkteam.com>
>>        */
>>       public function getPackageXML() {
>>
>> What's wrong?
>> You write "Return package meta as XML string". That's correct. But  
>> then the
>> method name should be
>> getPackageMetaXML() or better: call it "toXML()".
>>
>
> Yeay, toXML() is better. Would you leave the fromXML and toXML methods
> in the Package class when refactoring the actual XML processing out to
> another class?

Better extract it into another class.

>> 8) Checking for valid arguments
>>
>> You commented one setter with "@todo Only accept valid states".
>> That's true. But do it right away - you'll forget it otherwise.
>>
>> I added some checks for some methods, but there are a lot of  
>> methods left
>> which don't check their
>> arguments.
>>
>
> I'll have a look at that, but I think a test case should be present to
> test for the invalid arguments before adding checks. I'll do that for
> the obvious ones.

Yeah, do that. Believe me, these things help you in situations you
don't think of yet - like big refactorings ...

>> 12) Responsibilities
>>
> Mmmhh, I wasn't sure about that, too.
>
> I did the most obvious implementation by delegating the XML
> construction to the objects that know their properties and using the
> template pattern for extensibility. We could choose between a good
> separation of responsibilities or a more object-oriented design here.
>
> If striving for clean responsibilities it would be consequent to move
> the whole XML processing to separate MetaXMLReader / MetaXMLWriter
> classes. I'll think about that.
>
> That's it for now. I'll try to create a new patch with the improved
> code (at least my code) tonight.

Thanks a lot!

robert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PGP.sig
Type: application/pgp-signature
Size: 186 bytes
Desc: This is a digitally signed message part
Url : http://lists.netfielders.de/pipermail/typo3-team-core-v5/attachments/20080916/033196a9/attachment.pgp