Martin Jansen [2004-08-30 13:49 UTC] How does this package compare to the existing Image_Graph package? Do you see any chance to merge your efforts?
Jesper Veggerby [2004-08-30 14:02 UTC] As far as I can see things: GraPHPite has much more functionality than Image_Graph (i.e. more plottypes and datasets), plus it provides a more flexible way of customizing your plot (it is much more modular - again as far as I can see)
Helgi Þormar Þorbjörnsson [2004-08-30 19:14 UTC] Seems to me that Jesper is right, at least from what I've been looking at, so I really like to see this package in pear :)
Alexey Borzov [2004-08-30 20:45 UTC] Impressive!
Unfortunately, you may have a harder than usual time pushing this into PEAR as we already have (sort of) a graph package. Fortunately, your effort beats Image_Graph hands down so opposition should be minimal.
But before you have a chance to argue with "no competitive packages" defenders, you'll have to pass the CS police.
I didn't look too closely at the package but here are some obvious Coding Standards violations. E.g. file Image/GraPHPite/axis/axislogarithmic.inc contains class Image_AxisLogarithmic. This should be Image/GraPHPite/Axis/Logarithmic.php and Image_GraPHPite_Axis_Logarithmic respectively. Classes and functions declarations do not follow "one true brace" convention. Private methods and variables are not prefixed by underscores.
Also, your include.inc file loads almost *all* of the library, which is 800+ kb in size. Most PEAR packages implement some lazy loading scheme, where classes are loaded only when they are actually needed.
Jesper Veggerby [2004-08-30 21:13 UTC] I am aware of the issue with the "competing" package, but when I looked at the Image_Graph package I thought that my package (to use your words) beats Image_Graph - at least in functionality and versatility, which I think is what is required to be a useful graph tool (people don't want standard graphs - they want their own)
Thanks for clearing the CS part, I must admit the CS manual didn't clarify everything for me, but your comments surely helped alot - already had a "hard" time converting to this - so back on the horse :)
As regards to the include.inc file I agree that this is, if not a problem, then an . The issue is that it is not often possible to do lazy loading, since constructing the objects are left to the "user" when creating the graph. That is "I" do not know which classes are to be used. Of course it would be possible for at user to include only what he/she needs and for GraPHPite to include the required classes, but that (IMHO) requires for the user to have much higher knowledge of the package than is really needed.
As I hope you can see from this I have given some thought to it, but if anybody have *the* idea, comments are most welcome.
Thanks for your time!
Carsten Harnisch [2004-08-31 08:08 UTC] I worked a bit with the package already and the work is impressive. I hope to see GraPHPite in PEAR also.
Anyway a few thing are special, e.g. the "add-methods" often are returning the value, but also create a global-var internally. This might be good if you work directly in a PHP-file, if you do your work in a class or function this disturbs you a bit.
Jesper Veggerby [2004-08-31 10:35 UTC] Ah yes - the add "hack". I did that back in the early days because I had some problems with the PHP way of handling reference assignment versus copy-of assignment. I agree it is not a "standard" nor pretty way of doing things - but it works. This said: I will definite look into returning the objects instead of creating the global vars.
Thanks for your comment.
Sérgio Carvalho [2004-08-31 11:20 UTC] I've been taking it out for a spin. When E_NOTICE is enabled, the package emits a few warnings, thrashing the image generation. The most common offender is the test on the existence of gd2. Instead of: if ($GLOBALS['gd2']), it should be if (array_key_exists('gd2', $GLOBALS) && $GLOBALS['gd2'])
Since you'll be refactoring the package to deal with Coding Standards, take some time to run it with maximum error verbosity and clean it up.
Jesper Veggerby [2004-08-31 19:54 UTC] Had a problem with E_NOTICE problems in the past - thought I had overcome them, but seems like some are left - fixing them of course
Jesper Veggerby [2004-09-01 09:06 UTC] The ideas in the comments by Alexey Borzov, Carsten Harnisch and Sérgio Carvalho should now be implemented in the available package.
Alan Knowles [2004-09-01 09:24 UTC] Have you considered naming it
Image_Graphite, rather than Image_GraPHPite - It's not essential, but the PHP bit sticks out like a sore thumb in the list.
I havent really checked the CS stuff yet - the proposals needs fixing to show
require_once 'Image/Graphite.php', rather than then .inc stuff.
Alan Knowles [2004-09-01 09:38 UTC] OK - a bit more thorough CS list:
var $Canvas should be var $canvas (lcfirst variables)
most local vars should be lcfirst (i'm not sure if it's a CS standard, but it's common practice)
globals that are specific to the package should be named
$_IMAGE_GRAPHITE_* or $IMAGE_GRAPHITE_* depending if they are private/public globals.
you can probably put the named_colors.txt in a subdirectory and use dirname(__FILE__), rather than constants.
method names should be StudlyCaps, and not "perform_antialias()"
constants should be IMAGE_GRAPHITE_*, not just GRAPHITE_*
The folder layout needs fixing:
for example:
Image_Graphite_Grid should install into Image/Graphite/Grid.php
Image_Graphite_Grid_Lines should install into Image/Graphite/Grid/Lines.php
require_once / includes should not have ()'s they are statements, not methods
Image_GraPHPite_Parent may be better called Image_GraPHPite_Common, to be consitant with other packages.
Actually make the changes 'yet' is not really a hinderence to the voting, they could be made before the first release.
Looks like a pretty comprehensive package.
Jesper Veggerby [2004-09-01 10:04 UTC] Thanks for clearing this - during refacoring I *was* wondering whether fx the file Grid.php should be in Images/GraPHPite/Grid or just /Images/GraPHPite.
In my code (and the one available for download) I cannot find any instances of $Canvas (they should be lc first already!)
As regards to the require_once(), the CS manual states:
Note: include_once() and require_once() are statements, not functions. You don't *need* parentheses around the filename to be included.
But if it *is* required I will surely change it.
As regards to named_colors.txt - shouldn't it be placed in the data folder?
Image_GraPHPite_Parent -> Image_GraPHPite_Common definitely (more meaningfull)
I will change the globals. The CS manual states
If your package needs to define global variables, their name should start with a single underscore followed by the package name and another underscore. For example, the PEAR package uses a global variable called $_PEAR_destructor_object_list.
Which I originally read as $_GraPHPite_*, but I can understand the $_Image_GraPHPite. If it has to be uppercase: then sure - PEAR is of course uc already so maybe the example in the manual is misleading(?). About the private versus public globals: I can understand the difference, i.e. the $_GraPHPite_gd2 should have no meaning for the user but the $_GraPHPite_font should, but the manual clearly states that globals should be $_ always.
Could someone please second Alan's comment? I will surely change it but not if the change actually violates CS.
The other things I will change ASAP :)
|
