| Helgi Žormar 2004-11-01, 3:56 pm |
| Hi,
I had a quick look over, saw nothing big really, but some comments here:
include_once had (), but that should be okey, but most of the time we
remove those.
----
(Graph.php)
if (isset($params['left'])) {
$left = $params['left'];
} else {
$left = 0;
}
I my self prefere to see:
$left = 0;
if (isset($params['left'])) {
$left = $params['left'];
}
Well of course defaulting everything at the top, not before each if
sentence
----
$foo+$bar $bar-1 and etc. there should be a space before and after - +
and such, $foo + $bar, it's nicer to read it that way.
----
Some param comments are like 200 chars or some, it's okey to split them
up in lines if the comments are long.
----
(Graph.php)
switch ($class) {
case 'graph': $class = 'Image_Graph'; break;
case 'plotarea': $class = 'Image_Graph_Plotarea'; break;
Things like these, they are okey, but less readable, would recommend
fixing that even though it means more lines :)
----
(Graph.php)
switch (count($params)) {
case 1: return new $class($params[0]);
Same with this one as above, even in the lines below this, you'll get
massive long lines, which isn't CS of course.
(this is happening in more places but those were the most notable)
----
(Axis.php)
}
elseif ((($span) / $interval) > 10) {
$interval = $interval * 2;
}
This is a CS violation. Didn't notice it in many places but it's there.
----
I only picked random files and looked over, this package it to big to go
through all the files ;)
All the Dir and Class structure seems right to me, few CS issues,
readability issue, but in general I'm very happy with this package.
On Mon, 2004-11-01 at 16:27, Stefan Neufeind, PEAR wrote:
> Dear PEAR-qa,
>
> since we've not received too much feedback regarding our query to please
> review that dev-release of the new Image_Graph on pear-dev, we'd be
> happy if you could have a look.
>
> The latest source should now be fully in CVS, if I didn't forget
> anything. Only thing is that package.xml is not yet up to date, since it
> has just been extracted from the released package. I already moved
> README and LICENSE to the docs-dir upon checkin, but this is also not
> reflected in package.xml currently.
>
> Could you please have a look at the code, class- and file-structure and
> give feedback?
>
>
> Kind regards,
> Stefan
|