Matthew Fonda [2009-09-03 19:44 UTC] Looks great so far! A few minor comments:
- Image_Diff::handleFileErrors() could probably be protected instead of private
- In Image_Diff::nearlySame(), it may be better to check if the parameters are gd resources instead of checking if they are string, that way at the point where you first call gd functions you are 100% sure you are passing them a valid image resource.
- It might be nice to return the measure of difference instead of just true/false
On a side note, the ImageMagick 'compare' utility has some cool features such as outputting an image highlighting the differences between the two images being compared. I'm not sure how difficult that would be to implement, but it could be a cool feature to have!
Daniel O'Connor [2009-09-04 05:57 UTC] Is there any compelling reason to make these static methods?
Being static means I can't mock them out, and makes them more like namespaced functions than an Image_Diffing object.
Ie:
Image_Diff::nearlySame()
could easily be
$diff = new Image_Diff();
$diff->nearlySame();
while having none of the drawbacks of statics (http://misko.hevery.com/code-reviewers-guide/flaw-brittle-global-state-singletons/)
Michael Gauthier [2010-01-17 05:10 UTC] Looks good. A few comments:
1.) Like Daniel mentioned, making all the methods static makes it harder to unit test the class.
2.) You don't need to assign copyright to the PHP Group. It can be copyrighted to you or the company you work for.
3.) It would be nice to see some sort of high-level documentation in the source about how exactly the images are being compared. Is it just comparing all pixels to see if they are the same color? How is "near" defined, etc.
Christian Weiske [2010-12-22 11:01 UTC] any news?
Michael Gauthier [2011-01-30 21:40 UTC] Are you still trying to get this accepted as a PEAR package?
|
