Resizing and recoloring a logo using PHP GD Library

Question

I have the following code, that works very well. However I'm about to launch it to 5000 users and will no doubt take a beating early on. I want to ensure that if there are any opportunities to streamline so the code executes better or faster, that I do not miss them.

I've only pasted the PHP GD portion of the code, as the rest of my page is fine (some basic HTML)

$position = strpos($size, 'x');
$width = substr($size,0,$position);
$height = substr($size,$position+1);

$logo_image = imagecreatefrompng($image);
imagealphablending( $logo_image, false );
imagesavealpha( $logo_image, true );

$logo_width = imagesx($logo_image);
$logo_height = imagesy($logo_image);

if(($width>=601)&&($width<=1080)) {
    $percent = $width/$logo_width;
} elseif($width>=1081) {
    $percent = 1.2;
    $font_size = 28;
}

$logo_new_width = $logo_width*$percent;
$logo_new_height = $logo_height*$percent;

$base_image = imagecreatetruecolor($width, $height);

$logo_x = (($width/2)-($logo_new_width/2));
$logo_y = (($height/2)-($logo_new_height/2));

$text_box = imagettfbbox($font_size,$angle,$font,$text);
$text_width = $text_box[2]-$text_box[0];
$text_height = $text_box[3]-$text_box[1];

$x = ($width/2) - ($text_width/2);
if($height==768) {
    $y = (($height/2)+($logo_new_height/2));
} else {
    $y = (($height/2)+($logo_new_height));
}

//set overall background color here based on user selection
if($theme=='plum') {
    $fill_color = imagecolorallocate($base_image, 93, 62, 93);
} elseif($theme=='black') {
    $fill_color = imagecolorallocate($base_image, 0, 0, 0);
} elseif($theme=='white') {
    $fill_color = imagecolorallocate($base_image,255,255,255);
} elseif($theme=='gray') {
    $fill_color = imagecolorallocate($base_image,149,150,153);
} elseif($theme=='charcoal') {
    $fill_color = imagecolorallocate($base_image,78,80,84);
} elseif($theme=='warm') {
    $fill_color = imagecolorallocate($base_image,255,79,31);
}

//Set the Text Color    
if($textC=='black') {
    $text_color = imagecolorallocate($base_image,0,0,0);
} elseif($textC=='red') {
    $text_color= imagecolorallocate($base_image,255,79,31);
} elseif($textC=='white') {
    $text_color = imagecolorallocate($base_image,255,255,255);
} elseif($textC=='plum') {
    $text_color = imagecolorallocate($base_image, 93, 62, 93);
} elseif($textC=='gray') {
    $text_color = imagecolorallocate($base_image,78,80,84);
}

imagefill($base_image, 0, 0, $fill_color);
imagesavealpha($base_image, TRUE);

//set text color here
imagettftext($base_image,$font_size+1,0,$x,$y,$text_color,$font,$text );

imagecopyresampled($base_image, $logo_image, $logo_x, $logo_y, 0, 0,$logo_new_width, $logo_new_height, $logo_width, $logo_height);

$num_str = "";
$charsRand = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
for($i=0;$i<6;$i++)
    $num_str.=substr($charsRand,rand(0,strlen($charsRand)),1);

$filename_create = substr($text,0,strpos($text, ".")).'_'.$num_str.'.png';
imagepng($base_image,'images_created/'.$filename_create,1);
$file_size = filesize('images_created/'.$filename_create);

Did I do anything that will cause me grief? Any tips for me to optimize this portion of the code ?

Look forward to any and all comments. This was my first go around with GD.

Answer 1

If you have ImageMagick available, I would strongly consider moving towards that library - not necessarily for any performance/quality gains (I think this can vary between libraries based on use case) - but because it presents an object oriented interface. Right now, your code is very much procedural in nature and you might find it harder to maintain and reuse over time.

---

From a performance standpoint, your best best is to actually load test your application to see if it is going to meet your needs. I am guessing when you say 5000 users, you don't mean 500 concurrent users. How many users do you truly expect executing this script concurrently? How much memory does this take on typical execution? How does that compare against your server hardware? Is the user experience on your hardware (i.e. the time that this takes to execute) appropriate? Does that time to complete change under concurrent load?

Really your performance is going to have more to do with the questions I just asked than your code.

---

You need to DRY (Don't Repeat Yourself) up your code. You can do this through some configuration and use of variables:

For example:

$background_colors = [
    'plum' => ['red' => 93, 'green' => 62, 'blue' => 93],
    ...
];
$text_colors = [
    'black' => ['red' => 0, 'green' => 0, 'blue' => 0],
    ...
];

$fill_color = imagecolorallocate(
    $base_image,
    $background_colors[$theme]['red'],
    $background_colors[$theme]['green'],
    $background_colors[$theme]['blue'],
);

$text_color = imagecolorallocate(
    $base_image,
    $background_colors[$textC]['red'],
    $background_colors[$textC]['green'],
    $background_colors[$textC]['blue'],
);

This rids you of all those if-else conditionals. And also gives you the ability to lookup a provided background/text color string to determine if it exists in configuration. This could be useful in setting defaults. For example:

if(!empty($background_colors[$theme])) {
    $theme = $default_theme;
}

---

You have a lot of hard-coded "configuration" values (the colors as noted before, image size thresholds, font size, file name string length, etc.) that are simply hard-coded into your script. Personally, I would look to do all of this in a class context, so you have means to store default /overridden configuration as well as to get your main logic out of the main script path and provide you greater re-use down the line (like when you want to do this same thing on another site).

By the way, your $font_size variable only seems to be defined on one half of a conditional which could be problematic (though perhaps it is doe that is not shown).

---

You should actively try to design away else constructs. When you get in the habit of doing this, you will find that in most cases these are not really necessary.

Why should you do this? Because the more code paths you have, the more fragile and harder to maintain/debug your code will tend to be (the term cyclomatic complexity is used to describe this).

So when you see yourself with a conditional like this:

if(($width>=601)&&($width<=1080)) {
    $percent = $width/$logo_width;
} elseif($width>=1081) {
    $percent = 1.2;
    $font_size = 28;
}

Consider changing it to something like:

$percent = 1.2;
$font_size = 28;
if(($width>=601)&&($width<=1080)) {
    $percent = $width/$logo_width;
}

This is easier to read and reduces your potential code paths by one. This is also at the heart of my early comment to get rid of all those if-else conditions. They simply are not needed in this case and can be designed away.

Similarly,

if($height==768) {
    $y = (($height/2)+($logo_new_height/2));
} else {
    $y = (($height/2)+($logo_new_height));
}

Could become:

  $y = (($height/2)+($logo_new_height));
  if($height==768) {
    $y = (($height/2)+($logo_new_height/2));
  }

---

if($height==768) {

Do you really have a use case for this specific height? Should this be > or < comparison?

---

You should consider using exact comparisons (=== and !==) as your default means for comparison as opposed to the loose comparisons you are doing. Loose comparisons tend to introduce bugs into code around unexpected truthy/falsey conditions. I find myself using them in only very specific cases where a truly loose comparison is warranted.

---

Some stylistic thoughts:

• Use space around your comparison operators for better readability.
• Be consistent in using curly braces around flow-control structures (conditionals, loops, etc.).
• Try to limit the length of your lines to less than 80 characters to make the code easier to read. Your imagecopyresampled() call is pretty long and could easily be broken across multiple lines for better readability.