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

[Robert Lemke]

Hi Fthreeeprr team,

Am 10.09.2008 um 17:37 schrieb Christopher Hlubek:

> we finally made our code sprint for the first F3PR features in FLOW3.
>
> The results can be found in the F3PR Wiki on Forge:
> http://forge.typo3.org/wiki/flow3-f3pr/Sprint-0
>
> Can someone have a look at our patch (the last one on the wiki page)?
> None of us has SVN commit access so far but we need to commit these
> changes to the FLOW3 Package.

I reviewed your diff today - or at least part of it. I took a bit longer
than I expected, also because I couldn't apply the diff so easily  
anymore
due to our introduction of namespaces.

I uploaded a diff with my work to the sprint wiki (see above). I'm  
afraid
that you'll also have problems to apply it on your version but I hope  
that
you at least get enough useful information from it to complete the task.

The part I mostly looked at was Christophers Meta classes but I also  
briefly
looked at Tobis and Thomas code.

In general it looks really good and I'm reall, really happy to see  
that new
functionality in FLOW3. What's missing now is taking the last step and  
make
it clean enough so we can commit it to SVN ...

I suggest that you read the following comments and then go through the  
code
again and make it run with the latest SVN version. If you need any help
during that run, just try to reach Karsten or me ... somehow ;-)

When you're done with that, please send me another patch and I'll try to
review it as quick as possible.

So here are my comments (and much more could be said ;-)):

Cheers,
robert

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.

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?

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) {

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()".

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.

6) CGL in general.

We put a space between if (), switch (), foreach () etc.
See http://flow3.typo3.org/documentation/coding-guidelines/

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.

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.

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

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) {

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

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.


-------------- 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/20080915/4b6b41a0/attachment.pgp