Finding similarities between images

Question

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;
    }
  }

?>

Answer 1

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
//     including your `findSimilarityBetweenImages' function.
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.

Function signature comments can be useful as well (what nibra's answer was about).

Answer 2

Since your question is about coding style, consider to adhere to PSR-2.

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);
}

Answer 3

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);
  }

?>

Answer 4

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.