Re: [PEPr] +1 for PHP::UML
| From: | Baptiste Autin | Date: | Wed, 30 Apr 2008 13:15:14 +0000 |
| Subject: | Re: [PEPr] +1 for PHP::UML | ||
| References: | 1 | Groups: | php.pear.dev |
| Request: | Send a blank email to pear-dev+get-49945@lists.php.net to get a copy of this message | ||
I don't quite agree with your comment, and here is why:
> While the code itself looks good,
Thank you
> you really need to work on the docblocks
> since most methods miss them.
I would not say "most methods". The main class (PHP_UML), for example, has all
its methods commented.
You probably talk about PHP_UML_XMI_Factory2, which is a subclass of
PHP_UML_Factory.
Since its methods are 99% the same as PHP_UML_XMI_Factory1's,
I did not comment them.
What I would suggest to do, perhaps, is abstracting all methods in the parent
class, and commenting only the latter.
I could as well use an interface, as it is common practice in Java -
where all interfaces are commented, but the implementation classes very few.
> Your factory classes should be named PHP_UML_XMI_Factory_v1, not
> PHP_UML_XMI_Factory1 and *2 as they are now.
According to which rule ?
I purposely added that number (the OMG's XMI version) at the end of the
classname *without* any underscore, since the _ tends to become a package
separator, although this is still not quite formulated in those words in the CS.
In fact, the current CS - that I know well now - states that:
"The PEAR class hierarchy is also reflected in the class name, each level of the
hierarchy separated with a single underscore"
But PHP_UML_XMI_Factory1 and PHP_UML_XMI_Factory2 belong to the same "level of
hierarchy".
They are two implementation classes of an abstract class (PHP_UML_XMI_Factory),
and I do consider that those three classes should belong to the same package.
> This should also be reflected in
> the file layout.
I don't even want to think of it.
> Constructs like
> <?php
> else
> throw new PHP_UML_Exception('File '.$filename.' does not exist.');
> ?>
> are not allowed, you need {} around the block.
Ok, I'll fix that one.
Nevertheless, let me remind the CS to everybody:
"You are strongly encouraged to always use curly braces."
It does not say:
"You must always use curly braces."
However, if the PEAR community considers that this is an obligation, the
relevant coding rule must be updated conformingly.
In a more general manner, although I am aware of its utility, that PEAR coding
standard, I'm afraid of it, is not far from being a nightmare to many
developers, who might be used to different personal, or professional coding
rules, as you know.
As this is my first attempt to write a PEAR package, I spent a lot of time,
correcting my own code to adapt it to the PEAR CS. I was particulary annoyed by
spaces instead of tabs, and I find very upsetting stuff like "} else {", all on
the same line.
Yet I fully respected those "space and indentation rules", thanks to
PHPCodeSniffer (otherwise, if I had had to check everything by myself, I think I
would have given up trying to contribute from long).
But please, leave some air to the developers.
Enforcing documentation, E_STRICT-compatible code, file formats, is sound and
sensible, and I will do my best to add more method docblocks to PHP_UML (esp. to
the parser, which is complex)
But putting a conditional vote on a long package, because of a forgotten curly
brace, and of an incorrect class naming - which turns out to be, in fact, not
incorrect, but fully justified - is a bit excessive, IMO.
Baptiste Autin