2
\$\begingroup\$

This is a script I made that opens img.png, downloads the JSON data from the API, writes it on image and displays it. It currently is a community ad over Arqade.

<?php
header('Content-type: image/png'); //setting header to png so it will display as an image
$donationImage = imagecreatefrompng("img.png"); //loading the donation image's base from the file img.png
$white = imagecolorallocate($donationImage, 255, 255, 255);

$json = json_decode(utf8_encode(file_get_contents("http://www.mariomarathon.com/rest/partners/arqade.com")));
$total = round($json->total); //Rounding the total amount because it uses floats
$byus = $json->domainTotal;

imagettftext($donationImage,30,0,60,450,$white,'arial.ttf',"$".$total); //writing the total count to x:60 y:450
imagettftext($donationImage,30,0,370,450,$white,'arial.ttf',"$".$byus); //writing the donations by arqade.com to x:370 y:450
imagepng($donationImage); //displaying image
imagedestroy($donationImage); //removing image from memory
?>

Code and additional resources are available at GitHub. This file is the one named mariobeta.php.

How can I improve this code? I'm not that good with PHP but I think that the code doesn't have any major issues.

\$\endgroup\$
  • \$\begingroup\$ The code seems perfectly adequate for this very specific job. You could use imagettfbbox() to center the text, even if it varies in length. \$\endgroup\$ – KIKO Software Jun 17 '16 at 9:47
  • \$\begingroup\$ @KIKOSoftware thanks for the tip. Will be applying to live soon (and will experiment with imagettfbbox()) \$\endgroup\$ – Ave Jun 17 '16 at 12:10
1
\$\begingroup\$
header('Content-type: image/png');
imagepng($donationImage);

No real point in specifying the content type prior to actually outputting the image.

As mentioned in the other answer, json_decode(utf8_encode(file_get_contents("URI"))) can error out in multiple ways. If error reporting is active, this will result in a mismatch between content type and the response body. Even if it isn't, you end up sending a wrong content type for an empty body.


I hope you have placed a cache in front of that script. You are stalling the script with a REST query, and the memory footprint isn't negligible either.

If the web server isn't configured to cache, you want to fix that.

Caching the output with plain PHP is slightly more complex than that, with regard to acquiring a mutex on the cache during updates.


?>

Eventually you will end up with a newline following the closing code tag (be it the IDE or the VCS software being "smart"). This will add to the output, resulting in an invalid response.

Avoid using the closing code tag if there isn't any content following. This actually applies to all PHP files.

\$\endgroup\$
  • \$\begingroup\$ Fixed - The server is/was configured to cache, both on cloudflare and server ends. - Fixed. \$\endgroup\$ – Ave Sep 15 '16 at 20:07
0
\$\begingroup\$

To me, there are no significant problems in the code logic. I would however consider the following:

  • Try to get the image and JSON content first before you do anything else. Right now, you are just assuming that these steps work. If something goes wrong with retrieving either pieces of content, you are basically at a point where you don't have enough information to serve up the resulting image, so why would you have already sent an content-type header to client to make the client expect an image. You should send this header only when you know you will be sending an image response (as opposed to some error response).
  • Similarly to above, you are just assuming "happy path" execution throughout. What happens if you get malformed JSON from the remote server and json_encode() doesn't work? What happens if any of your image manipulation steps fail? You should generally understand all possible return values from an operation and handle each possible result, both "happy path" and error conditions.
\$\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.