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

[Christopher Hlubek]

Hi,

I couldn't finish my work on the patch yesterday. Using the PDT with
all those namespace errors isn't very comfortable. And there's a lot
of refactoring to be done. I'll try to finish it until the weekend,
that should be more realistic.

Greetings,
Christopher

2008/9/16 Robert Lemke <robert at typo3.org>:
> 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
>
> _______________________________________________
> TYPO3-team-core-v5 mailing list
> TYPO3-team-core-v5 at lists.netfielders.de
> http://lists.netfielders.de/cgi-bin/mailman/listinfo/typo3-team-core-v5
>
>