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

[Christopher Hlubek]

Hi Robert,

I have implemented the namespace notation into our changes and will
fix up some of the things you mentioned today.


2008/9/15 Robert Lemke <robert at typo3.org>:
> 1) Find the right test case for your tests.
>
> In the MetaTest test case you implemented a test "getPackageMetaWorks()".
> This test is, in reality,
> a functional test which checks if the getPackageMeta() method of the Package
> object returns a Meta
> object and if that Meta object contains the right information.
>
> I know that I also tended to write tests of this kind but now I learned that
> we really have to
> make sure writing a) unit tests and b) only test one class at a time (if
> possible).
>
> I created a new test getPackageMetaReturnsAMetaObject() in the Package test
> case and modified your
> tests in the Meta test case accordingly.
>
>

I think I'll refactor the meta XML reading and writing to a new class
that i'll test separately.

> 2) Find a good method name for your tests.
>
> By (undocumented) convention, the test names should describe your
> assumptions about the code
> you test. "getPackageMetaWorks()" is too fuzzy - what do you expect that
> method to do in order
> to work?
>

That's true, for the first tests I used the naming schema you had
originally in the PackageTest testcase. I'll refactor the names to be
more descriptive.

> 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?

> 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?

> 5) Inline comments.
>
> We hardly ever use inline comments because
>
>   - either the code is self explanatory anyway
>   - or the code block in question can be extracted into its own method with
> a meaningful method name
>
> If you really really need to comment something inline, the rule is to ident
> the comment:
>
>                // The following complex code echoes some text:
>        echo('some text);
>
> In general please don't comment the end of loops, conditions etc.
>

I'll get rid of those ;)

> 7) Use constants for magic strings / numbers
>
> For magic strings such as "Alpha", "Beta" etc. it is very important to
> provide constants which can be
> used elsewhere:
>
> F3::FLOW3::Package::Meta::STATE_ALPHA
>
> The same goes for the constraint type.
>

Okay, I thought about that but didn't implement it right away. I'll
externalize the states and constraint types.

> 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.

> 9) Method names
>
> For me the method name getConstraints() implies that I can get _all_
> constraints. So it would
> be better to make the $constraintType optional:
>
> public function getConstraints($constraintType = NULL) {
>        if ($constraintType === NULL) return $this->constraints;
>        ...
>

Yeah, that's more convient.

> 10) Type Hinting
>
> /**
>  * @param F3_FLOW3_Package_Meta_Constraint $constraint
>  */
> public function addConstraint($constraint) {
>        $this->constraints[$constraint->getConstraintType()][] = $constraint;
> }
>
> Although you expect a contstraint as a parameter, you forgot to add the type
> hint to the method
> signature:
>
> public function addConstraint(F3::FLOW3::Package::Meta::Constraint
> $constraint) {
>

Must have forgotten that one.

> 11) Complete Doc Comments
>
> In many methods the @return tag is missing, any some others (mostly setters
> / getters) are lacking
> the general description. Some parameters are undocumented.
> I know, it's trivial, but we want full documentation for all methods ..
>

I'll add the missing doc comments.

> 12) Responsibilities
>
> In the class F3::FLOW3::Package::Meta::Party there's a method
> "writePartyXML()". I didn't get on the
> first glance what the purpose of that method does but when I found it out, I
> felt that it's not
> the responsibility of the Meta::Party object to add XML to some XMLwriter
> (same goes for the
> writeXML method in the abstract Party class)
>
> Instead I suggest that the Meta::* classes only contain information and
> logic. Consider the XML as
> the view - it doesn't belong into the model. It's better to have a method
> which extracts all the
> information from the different meta objects and transforms it into XML.
>

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.

Greetings,
Christopher