Finding similarities between images

I've posted here before and got pretty helpful reviews. How does my code look now? To keep it really helpful, I just copied this from the middle of one of my projects.

<?php

function findSimilarityBetweenImages($imagePathA,$imagePathB, $accuracy){ //if images are png$imageA = imagecreatefrompng($imagePathA);$imageB = imagecreatefrompng($imagePathB); //if images are jpeg //$imageA = imagecreatefromjpeg($imagePathA); //$imageB = imagecreatefromjpeg($imagePathB); //get image resolution$imageX = imagesx($imageA);$imageY = imagesy($imageA);$pointsX = $accuracy*5;$pointsY = $accuracy*5;$sizeX = round($imageX/$pointsX);
$sizeY = round($imageY/$pointsY); //Compare the color of each point while looping through.$y = 0;
$match = 0;$num = 0;
for ($i=0;$i < $pointsY;$i++) {
$x = 0; for($n=0; $n <$pointsX; $n++){$rgba = imagecolorat($imageA,$x, $y);$colorsa = imagecolorsforindex($imageA,$rgba);

$rgbb = imagecolorat($imageB, $x,$y);
$colorsb = imagecolorsforindex($imageB, $rgbb); if(colorComp($colorsa['red'], $colorsb['red']) && colorComp($colorsa['green'], $colorsb['green']) && colorComp($colorsa['blue'], $colorsb['blue'])){$match ++; //Match between points.
}
$x +=$sizeX;
$num++; }$y += $sizeY; } //A similarity or percentage of match between the two images.$similarity = $match*(100/$num);
return $similarity; } function colorComp($color, $c){ //To see if the points match if($color >= $c-2 &&$color <= $c+2) { return true; } else { return false; } } ?> 4 Answers Ok, so I have made lots of changes to the code. I have left comments starting with >>>'s all over the code to note what I have changed and why. I didn't really look into what the code is for and there could probably be more things I would like to have changed if I did. For some of the parts I didn't understand I checked The Pattern in Pi's code, thanks. I have made some style changes along the way, I generally follow the PHP Framework Interop Group's standards. And now the code: <?php define('I_DONT_KNOW_WHAT_THIS_NUMBER_IS', 5); // >>> I have made it so that the images are passed in as resources // this makes the function more flexible and might stop you from // needing to read the images from disk at a later point // >>> Hmm, I have also changed the name of this function as it doesn't 'find' so much as 'measure' // >>> Ok, so I have added some phpdoc style commenting to this function // It is important here and not in the helper functions as the idea behind this function would // otherwise seem vague. /** * This measures the similarity between two images. * It does so by sampling the image at certain points (determined by the accuracy * setting) and comparing the rgb color values for an almost exact match (See COLOR_COMPARISON_FUZZYNESS). The number of * matches per number of samples determines the similarity. * * @arg$imageA   Resource An image to compare, should be an image resource
* @arg $imageB Resource An image to compare, should be an image resource * @arg$accuracy Integer  This number gets timesed by 5 for some reason and then becomes the number of samples per a dimension
*
* @returns float The ratio between the number of matcher per the number of samples for the two images
*/
function measureSimilarityBetweenImages($imageA,$imageB, $accuracy) { // >>> I don't understand this part at all... If you explain it I will check it out // >>> Ok after looking at @The Pattern in Pi's answer I sort of understand, // I also couldn't help but use some of his variable names, thanks // >>> Note that I have changed this comment // Get the image size of the first image and assume it is the same as the second$imageWidth = imagesx($imageA);$imageHeight = imagesy($imageA); // >>> What was this 5, magic numbers should be constants! // >>> I still don't know what density means in this context // >>> Ok, I know know what density means, I would call it numberOfSamples // density normally means means units per (some other unit)^3 // and have it be the total number of samples over the image // but I still don't know where accuracy and the 5 comes in$samplesPerDimension = $accuracy * I_DONT_KNOW_WHAT_THIS_NUMBER_IS; // >>> I like to use units as variable names$xPixelsPerSample = round($imageWidth/$samplesPerDimension);
$yPixelsPerSample = round($imageHeight/$samplesPerDimension); // >>> This is not the best way to do this but it should work, a // better solution might involve changing the above 'round' function // to a 'floor'. Be careful with rounding.$totalSamples = floor($xPixelsPerSample *$imageWidth) * floor($yPixels *$imageHeight);

// >>> Again I don't understand this part, these loops look very convoluted
// >>> After looking at The Pattern in Pi's code, again I sort of understand
//     again I sort of can't help use his code...
// >>> Actually on second thought, it doesn't look like pointsWidth or
//     pointsHeight is defined anywhere in his revision...
// >>> EDIT: ok I have fixed this up
// >>> I have changed this name so it is clearer what it represents
$matchedSamples = 0; // >>> Note how I have inlined these loops for ($y = 0; $y <$imageHeight; $y =+$yPixelsPerSample) {
for ($x = 0;$x < $imageWidth;$x =+ $xPixelsPerSample) { // >>> So I cleaned up this part through the use of functions below if (comparePixels(getPixel($imageA, $x,$y), getPixel($imageB,$x, $y))) {$matchedSamples++; //Match between points.
}
}
}

// >>> I like to leave ratios alone, it can be converted to a percentage when it needs to be output
return $matchedSamples/$totalSamples;
}

// Composing these two functions together
function getPixel($image,$x, $y) { return imagecolorsforindex($image, imagecolorat($image,$x, $y)); } // >>> With compare functions I like to just use$a and $b, this is just a preference of mine function comparePixels($a, $b) { // >>> Note the usage of the array and the foreach loop foreach (array('red', 'blue', 'green') as$color) {
if (! compareColors($a[$color], $b[$color])) {
return false;
}
}
return true;
}

// >>> Constants are normally together at the top (or in a separate parameters file) but I have put it here for effect
define('COLOR_COMPARISON_FUZZYNESS', 2)

// >>> I changed this functions name, this is largely a matter of preference and consistancy.
//     The pattern I use here seems to be verb then noun, see how it matches all the other functions
function compareColors($a,$b)
{
// >>> I have clarified what you are doing here, it shows that the difference between the two values
//     must be less a certain amount, which I have made a constant
if(abs($a -$b) <= COLOR_COMPARISON_FUZZYNESS)) {
return true;
}
return false;
}

// >>> So you would call it like this to get the same effect
measureSimilarityBetweenImages(imagecreatefrompng($imagePathA), imagecreatefrompng($imagePathB));

?>

Some tips:

Keep in mind magic numbers and subtle naming patterns.

Checkout functional programming (it is not just programing using functions), at least the philosophy/paradigm if not a functional language (such as haskel and clojure). Functional programming is definitely not mutually exclusive to object orientated programming, they both can work together.

Your comments didn't prove very useful to me, important things to comment about is the domain specific things. It would be useful for you to explain how you are finding the similarities and what you define a similarity is, I suspect that is where density comes in.

I don't normally comment on what code is doing, I try to make the code readable enough so that I don't have to, in the very rare cases I can't make it readable I put comments in. There is also a style of coding called literate programming which is the opposite of this.

• @ThePatternInPi has made some suggestions as to what the code does and I'll make some fixes. May 7 '13 at 15:04
• This code could have been more functional by having the sampling part of the code seperate from the pixel comparison in some kind of booleanSampleImages($imageA,$imageB, $comparePixels,$pixelsPerSample) function. May 7 '13 at 15:49
• Great Explanation! May 7 '13 at 16:00

Your color comparision function is bloated. 'If the expression is true, then return true, else, if the expression is false, then return false.' Just return the expression.

return ($color >=$c-2 && $color <=$c+2);

Next, I'd make the literal '2' variable, and add some DocBlock documentation:

/**
* Check if two color components are similar
*
* @param  int  $color The first color component's value * @param int$c      The second color component's value
* @param  int  $dist The maximum difference for two values to be considered equal * * @return boolean True, if the color components are similar. */ function colorComp($color, $c,$dist)
{
return ($color >=$c-$dist &&$color <= $c+$dist);
}
• Wow. The DocBlock improves the readability of my code even for me! Thanks! May 2 '13 at 18:46

Note: I seriously recommend scjr's response over my own. His code implements all of my (minor) adjustments, but also includes the improved coding practices, organization, and straightforward logic that good code should have.

Making it more functional and adjusting the variable names helps make it more readable and helps you more quickly identify what each part of code does what.

TotalPoints can be calculated, and doesn't belong in the loop. I combined x, i, n, and y, to clean up the loops as well. They could be adjusted even more, changing x++ to x+=xIncrement, removing the need to multiply x and y at every point (a performance hit that I introduced that you could remove).

<?php

function findSimilarityBetweenImages($imagePathA,$imagePathB, $accuracy){ //if images are png$imageA = imagecreatefrompng($imagePathA);$imageB = imagecreatefrompng($imagePathB); //if images are jpeg //$imageA = imagecreatefromjpeg($imagePathA); //$imageB = imagecreatefromjpeg($imagePathB); //get image resolution$imageWidth = imagesx($imageA);$imageHeight = imagesy($imageA);$density = $accuracy * 5;$xIncrement = round($imageWidth/$density);
$yIncrement = round($imageHeight/$density); //Compare the color of each point while looping through.$matchingPoints = 0;

for ($y=0;$y < $density;$y++) {
for ($x=0;$x < $density;$x++){

$colorsa = colorData($imageA, $x*$xIncrement, $y*$yIncrement);
$colorsb = colorData($imageB, $x*$xIncrement, $y*$yIncrement);

if(colorsMatch($colorsa,$colorsb)){
$matchingPoints++; //Match between points. } } }$totalPoints = $pointsHeight *$imageHeight;

//A similarity or percentage of match between the two images.
$similarity =$matchingPoints*(100/$totalPoints); return$similarity;
}

function colorData($imageA,$x, $y){$rgb = imagecolorat($imageA,$x, $y); return imagecolorsforindex($imageA, $rgb); } function colorsMatch($colorsa, $colorsb){ //Compare the R, G, and B values return colorComp($colorsa['red'], $colorsb['red']) && colorComp($colorsa['green'], $colorsb['green']) && colorComp($colorsa['blue'], $colorsb['blue']); } function colorComp($color, $c){ //To see if the points match return ($color >= $c-2 &&$color <= \$c+2);
}

?>

I suggest object oriented programming over the procedural style for many reasons I won't get into here. I also suggest being consistent about using camelCase throughout all of your code.