5
\$\begingroup\$

I am working on a site that themes itself based on images / logos. I have made a utility class that gets the colors from an image, shown below. I am fairly new to OOP with PHP, and I'm looking for feedback on what I could improve with the following. It is still in beta, so image-handling is a little sketchy and could use some work.

Image Used:

enter image description here

Class:

class Image {
    private $_hex = array();
    private $_size = array();
    private $_topThree = array();
    private $_im, $_mostCommon, $_minDem, $_tempHex, $_uniqueHex;
    
    public function __construct($image) {
        $ext = explode('.',$image);
        $ext = end($ext);
        if ($ext === 'png') {
            $this->_im = imagecreatefrompng($image);
        } elseif ($ext === 'jpg' || $ext === 'jpeg') {
            $this->_im = imagecreatefromjpeg($image);
        } else {
            die('The supplied extension is not supported. Supported formats include: jpg, jpeg, and png.');
        }
        $this->_size = getimagesize($image);
        $this->setHex();
    }
    
    public function showAll() {
        foreach($this->_uniqueHex as $k) {
            echo '<div style="background-color:'.$k.'; width:100%; height:30px;">'.$k.'</div>';
        }
    }
    
    public function getMostCommon() {
        $this->mostCommon($this->_hex);
        return $this->_mostCommon;
    }
    
    public function getTop() {
        $this->_tempHex = $this->_hex;
        $counted = array_count_values($this->_tempHex);
        arsort($counted);
        $i = 0;
        foreach ($counted as $k => $v) {
            if ($i < 3) {
                $this->_topThree[$i] = $k;
            }
            $i++;
        }
        return $this->_topThree;
    }
    
    public function showTop() {
        if (empty($this->_topThree)) {
            $this->getTop();
        }
        foreach($this->_topThree as $k) {
            echo '<div style="background-color:'.$k.'; width:100%; height:30px;">'.strtoupper($k).'</div>';
        }
    }
    
    #########################
    ### PRIVATE FUNCTIONS ###
    #########################
    private function setHex() {
        $x = 0;
        $y = 0;
        $this->_minDem = min($this->_size[0], $this->_size[1]);
        // Get RGB pixel by pixel
        while ($x < $this->_minDem) {
            $colors = imagecolorat($this->_im, $x, $y);
            $r = ($colors >> 16) & 0xFF;
            $g = ($colors >> 8) & 0xFF;
            $b = $colors & 0xFF;
            // Convert RGB to Hex
            $this->_hex[] = $this->toHex($r, $g, $b);
            $x++;
            $y++;
        }
        $this->removeWhiteBlack($this->_hex);
        $this->_uniqueHex = array_unique($this->_hex);
    }
    
    private function removeWhiteBlack($array) {
        $i = 0;
        foreach($array as $k) {
            $k = strtolower($k);
            if ($k === '#ffffff' || $k === '#000000') {
                unset($this->_hex[$i]);
            }
            $i++;
        }
    }
    
    private function mostCommon() { 
        $counted = array_count_values($this->_hex); 
        arsort($counted); 
        $this->_mostCommon = key($counted);   
    }
    
    private function toHex($r, $g=-1, $b=-1) {
        (is_array($r) && sizeof($r) == 3) ? list($r, $g, $b) = $r : NULL;
        $r = intval($r); $g = intval($g);
        $b = intval($b);
        $r = dechex($r<0?0:($r>255?255:$r));
        $g = dechex($g<0?0:($g>255?255:$g));
        $b = dechex($b<0?0:($b>255?255:$b));
        $color = (strlen($r) < 2?'0':'').$r;
        $color .= (strlen($g) < 2?'0':'').$g;
        $color .= (strlen($b) < 2?'0':'').$b;
        return '#'.$color;
    }
}

Usage:

$image = new Image('FGC.png');
$topThree = $image->getTop();
$image->showTop();
echo $image->getMostCommon();
echo '<pre>', print_r($topThree, true), '</pre>';
$image->showAll();

Output:

Top 3:

<div style="background-color:#155184; width:100%; height:30px;">#155184</div>
<div style="background-color:#009467; width:100%; height:30px;">#009467</div>
<div style="background-color:#dceae3; width:100%; height:30px;">#DCEAE3</div>

Most Common Color:

#155184

Top Three in array:

Array
(
    [0] => #155184
    [1] => #009467
    [2] => #dceae3
)

Unique:

<div style="background-color:#155184; width:100%; height:30px;">#155184</div>
<div style="background-color:#61749d; width:100%; height:30px;">#61749d</div>
<div style="background-color:#fffffd; width:100%; height:30px;">#fffffd</div>
<div style="background-color:#ede2d1; width:100%; height:30px;">#ede2d1</div>
<div style="background-color:#e7d6bd; width:100%; height:30px;">#e7d6bd</div>
<div style="background-color:#e2cdad; width:100%; height:30px;">#e2cdad</div>
<div style="background-color:#948e96; width:100%; height:30px;">#948e96</div>
<div style="background-color:#babfd3; width:100%; height:30px;">#babfd3</div>
<div style="background-color:#009467; width:100%; height:30px;">#009467</div>
<div style="background-color:#9b8d87; width:100%; height:30px;">#9b8d87</div>
<div style="background-color:#3da686; width:100%; height:30px;">#3da686</div>
<div style="background-color:#6d9974; width:100%; height:30px;">#6d9974</div>
<div style="background-color:#c0aa92; width:100%; height:30px;">#c0aa92</div>
<div style="background-color:#dceae3; width:100%; height:30px;">#dceae3</div>
<div style="background-color:#19986c; width:100%; height:30px;">#19986c</div>
<div style="background-color:#858880; width:100%; height:30px;">#858880</div>
<div style="background-color:#90947c; width:100%; height:30px;">#90947c</div>
<div style="background-color:#707782; width:100%; height:30px;">#707782</div>
\$\endgroup\$
6
  • 2
    \$\begingroup\$ See 99designs.com/tech-blog/blog/2012/05/11/color-analysis and github.com/99designs/colorific \$\endgroup\$ – Dave Jarvis Dec 19 '13 at 18:03
  • \$\begingroup\$ @DaveJarvis that's a pretty cool article \$\endgroup\$ – bumperbox Jan 6 '15 at 8:56
  • \$\begingroup\$ What if "most common" is a tie? What if there is a tie for 3rd most common? Are you happy to arbitrarily lose otherwise qualifying colors? Do you ever want to present the counts for each color to see weighting? I personally do not support declaration inside of ternaries or in other words, ternaries that do not begin with a variable declaration. \$\endgroup\$ – mickmackusa May 22 at 6:39
  • \$\begingroup\$ @mickmackusa this is all code I wrote when I first started as a developer 8+ years ago. I'd likely do it very differently now if I still wrote PHP. \$\endgroup\$ – jnthnjns May 22 at 19:52
  • \$\begingroup\$ I'm honestly surprised this is getting activity 8 years later. I was going to delete but I like Sam's answer so I'll give them the points :) \$\endgroup\$ – jnthnjns May 22 at 19:54
2
\$\begingroup\$

General Feedback

The class looks okay. I like how the method names follow PSR-12 recommendation of method names in camelCase, however it is also recommended that an underscore not be used to indicate private properties1

Property names MUST NOT be prefixed with a single underscore to indicate protected or private visibility. That is, an underscore prefix explicitly has no meaning.

The naming of some methods could be better - e.g. instead of getTop() a better name might be getTopThreeUsedColors - getTop() sounds like a method called to get something related to the top of the image. Maybe a better name for the class would be something like ImageColorAnalyzer.

PSR-5 is in draft status currently but is commonly followed amongst PHP developers. It recommends adding docblocks above structural elements like classes, methods, properties, etc. Many popular IDEs will index the docblocks and use them to suggest parameter names when using code that has been documented. At least do it for the public methods since outside code could reference the docblocks and provide hints for the parameters, return type, etc.

Suspected bug

The description states that the code "gets the colors from an image" yet given that there is a single while loop in the method Image::setHex() which increments $x and $y at the same time it appears that it really gets the color at points along a diagonal line - like this:

enter image description here

To get all of the image colors it would likely need to have two loops- one to increment $x and one to increment $y independently.

Suggestions

Check mime type

Instead of using the file extension to get the image type, check the MIME type using fileinfo_file() since a user could modify the extension, which may include a non-image files and that could lead to errors when the imagecreatefrom*() functions are called.

Getting top three colors

The method Image::getTop() iterates over all elements. Instead of looping, it could just take a slice of the array using array_slice().

If a loop was needed, it could loop over the keys (using array_keys($counted) to avoid the need to maintain $i separately

Abstract common styles

In the loop of Image::showAll() each <div> contains three styles, the latter two being redundant. Those could be moved out to a <style> tag - either with a ruleset for div or other selector like a class name. In case performance/bandwidth is a concern this can decrease the amount of data returned by ~23%.

Skip white and black

Instead of calling removeWhiteBlack() the code could simply not add white and black to the array.

Array syntax

There isn't anything wrong with using array() but as of the time of writing, PHP has Active support for versions 8.0 and 7.4, and since PHP 5.4 arrays can be declared with short array syntax (PHP 5.4) - i.e. [].

Converting RGB to Hex values

It looks like the code within Image::toHex() was pasted from an online source like this one. There are other techniques, many of which are shorter - e.g. using sprintf(). See answers to this SO post for simpler alternatives (except the answer by Rajarshi Das since that is nearly identical to the current code).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.