Formatting an if statement for readability

Question

What's the best way to format this for readability?

if (strpos($file, '.jpg',1) && file_exists("$thumbsdir/$file") == false || strpos($file, '.gif',1) && file_exists("$thumbsdir/$file") == false || strpos($file, '.png',1) && file_exists("$thumbsdir/$file") == false) {
  createThumb("$gallerydir/$file", "$thumbsdir/$file",$thumbsize);
  fwrite($log,date("Y-m-d")." @ ".date("H:i:s")."  CREATED: $thumbsdir/$file\n");
}

Neil Williams · Accepted Answer · 2009-12-04T19:27:23.780

I'd extract the "is an image" logic into its own function, which makes the if more readable and also allows you to centralize the logic.

function is_image($filename) {
    $image_extensions = array('png', 'gif', 'jpg');

    foreach ($image_extensions as $extension) 
        if (strrpos($filename, ".$extension") !== FALSE)
            return true;

    return false;
}

if (is_image($file) && !file_exists("$thumbsdir/$file")) {
    createThumb("$gallerydir/$file", "$thumbsdir/$file",$thumbsize);
    fwrite($log,date("Y-m-d")." @ ".date("H:i:s")."  CREATED: $thumbsdir/$file\n");
}

score 4 · Answer 2 · answered Oct 20 '08 at 07:24

4

if ((strpos($file, '.jpg',1) ||
     strpos($file, '.gif',1) ||
     strpos($file, '.png',1))
    && file_exists("$thumbsdir/$file") == false)
{
  createThumb("$gallerydir/$file", "$thumbsdir/$file",$thumbsize);
  fwrite($log,date("Y-m-d")." @ ".date("H:i:s")."  CREATED: $thumbsdir/$file\n");
}

answered Oct 20 '08 at 07:24

Fire Lancer

29,364
31
116
182

1

is there a particular reason to put the || ORs on the end of the line and the && ANDs at the start? – nickf Oct 20 '08 at 13:12
@nickf: If I had to guess, it has to do with not mixing up the parens, but definitely does more harm than good. – eyelidlessness Oct 25 '08 at 08:52

ConroyP · Answer 3 · 2008-10-20T07:42:15.863

2

The file_exists check seems to be constant for each of the file types, so don't compare them unless the file_exists check has been passed.

if (file_exists("$thumbsdir/$file") == false)
{
   if(strpos($file, '.jpg',1) ||
     strpos($file, '.gif',1) ||
     strpos($file, '.png',1)
   {
     createThumb("$gallerydir/$file", "$thumbsdir/$file",$thumbsize);
     fwrite($log,date("Y-m-d")." @ ".date("H:i:s")."  CREATED: $thumbsdir/$file\n");
   }
}

edited Oct 20 '08 at 07:42

answered Oct 20 '08 at 07:28

ConroyP

40,958
16
80
86

Wouldn't you want to do it the other way around because file_exists would be more expensive? – Neil Williams Oct 20 '08 at 07:30
@Neil: It would depend on how often the tests failed. I doubt the strpos() tests will fail often, so they won't cut down on the number of tests against file_exists(). However, the file_exists() test will fail whenver a thumbnail has already been generated, skipping the filename tests. – John Millikin Oct 20 '08 at 07:34

score 2 · Answer 4 · answered Oct 20 '08 at 07:28

function check_thumbnail($file)
{
    return (strpos($file, '.jpg',1) && file_exists("$thumbsdir/$file") == false ||
            strpos($file, '.gif',1) && file_exists("$thumbsdir/$file") == false ||
            strpos($file, '.png',1) && file_exists("$thumbsdir/$file") == false);
}

if (check_thumbnail ($file)) {
  createThumb("$gallerydir/$file", "$thumbsdir/$file",$thumbsize);
  fwrite($log,date("Y-m-d")." @ ".date("H:i:s")."  CREATED: $thumbsdir/$file\n");
}

After extracting the logic to a separate function, you can reduce the duplication:

function check_thumbnail($file)
{
    return (strpos($file, '.jpg',1) ||
            strpos($file, '.gif',1) ||
            strpos($file, '.png',1)) &&
           (file_exists("$thumbsdir/$file") == false);
}

check_thumbnail is not a good name - does a return value of true mean it exists or does not exist? Maybe is_existing_thumbnail() or non_existent_thumbnail() would be a better name - the former requires inverting the logic, of course. — Jonathan Leffler, Oct 20 '08 at 07:36

score 2 · Answer 5 · answered Oct 20 '08 at 07:31

2

I would seperate the ifs as there is some repeating code in there. Also I try to exit a routine as early as possible:

if (!strpos($file, '.jpg',1) && !strpos($file, '.gif',1) && !strpos($file, '.png',1))
{
    return;
}

if(file_exists("$thumbsdir/$file"))
{
    return;
}

createThumb("$gallerydir/$file", "$thumbsdir/$file",$thumbsize);
fwrite($log,date("Y-m-d")." @ ".date("H:i:s")."  CREATED: $thumbsdir/$file\n");

answered Oct 20 '08 at 07:31

Rob Bell

3,542
5
28
49

I agree with exiting a routine ASAP, I think I'll do that. – PHLAK Oct 20 '08 at 07:40

score 1 · Answer 6 · edited May 23 '17 at 12:04

I would break it up like this, setting aside the redundancy issue:

if (strpos($file, '.jpg',1) && file_exists("$thumbsdir/$file") == false
 || strpos($file, '.gif',1) && file_exists("$thumbsdir/$file") == false
 || strpos($file, '.png',1) && file_exists("$thumbsdir/$file") == false) {
  createThumb("$gallerydir/$file", "$thumbsdir/$file",$thumbsize);
  fwrite($log,date("Y-m-d")." @ ".date("H:i:s")."  CREATED: $thumbsdir/$file\n");
}

@Fire Lancer's answer addresses the redundancy well.

score 1 · Answer 7 · answered Oct 20 '08 at 16:29

I find the following to be more readable using getimagesize(). I'm writing this off the top of my head so it may require some debugging.

Vertical code is more readable than horizontal, imho.

// Extract image info if possible
    // Note: Error suppression is for missing file or non-image
if (@$imageInfo = getimagesize("{$thumbsdir}/{$file}")) {

    // Accept the following image types
    $acceptTypes = array(
        IMAGETYPE_JPEG,
        IMAGETYPE_GIF,
        IMAGETYPE_PNG,
    );

    // Proceed if image format is acceptable
    if (in_array($imageInfo[2], $acceptTypes)) {

        //createThumb(...);
        //fwrite(...);

    }

}

Peace + happy hacking.

score 1 · Answer 8 · answered Oct 25 '08 at 08:59

Might as well throw my two cents in.

if(!file_exists($thumbsdir . '/' . $file) && preg_match('/\.(?:jpe?g|png|gif)$/', $file)) {
    createThumb($gallerydir . '/' . $file, $thumbsdir . '/' . $file, $thumbsize);
    fwrite($log, date('Y-m-d @ H:i:s') . '  CREATED: ' . $thumbsdir . '/' . $file . "\n");
}

Formatting an if statement for readability

8 Answers8

Linked