Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My code looks like "spaghetti code". I have yet to use OOP in PHP but I am really interested in how a good programmer would handle this many if statements. It looks like a mess.

<?php
    session_start();
    include './functions.php';

    if(empty($_FILES['browseMovie']) || empty($_FILES['browseThumbnail']) || empty($_GET['title']) || empty($_GET['description']))
    {
        die('Please insert all fields.');
    }

    $filePartsMovie = pathinfo($_FILES['browseMovie']['name']);
    $filePartsThumbnail = pathinfo($_FILES['browseThumbnail']['name']);

    if($filePartsMovie['extension'] !== "mp4")
    {
        die('Video has to be a .mp4 file.');
    }

    if($filePartsThumbnail['extension'] !== "jpg" && $filePartsThumbnail['extension'] !== "png")
    {
        die('Thumbnail has to be in jpg/jpeg or png format.');
    }

    if($_FILES['browseMovie']['size'] + $_FILES['browseThumbnail']['size'] > 1600600000)
    {
        die('filesize of video and thumbnail = higher 1.5 GB.');
    }

    $id = $_SESSION["id"];
    $name = $_SESSION["username"];
    $folderName = generateUniqueFolder();
    $title = mysql_real_escape_string($_GET['title']);
    $description = mysql_real_escape_string($_GET['description']);

    // the original file name will be stored in the database as well
    $thumbFileName = basename($_FILES['browseThumbnail']['name']);
    $vidFileName = basename($_FILES['browseMovie']['name']);

    $userDir = "../users/$name/";
    $videoDir = $userDir . "videos/" . $folderName . "/";

    $uploadMovieDir =  $videoDir . $vidFileName;
    $uploadThumbnailDir = $videoDir . $thumbFileName;

    if(!isLoggedIn() || !is_dir($userDir))
    {
        die('User does not have own folder. Please contact the administrator.');
    }

    if(!mkdir($videoDir))
    {
        die('Could not create unique video folder.');
    }

    if(!move_uploaded_file($_FILES['browseMovie']['tmp_name'], $uploadMovieDir))
    {
        rmdir($videoDir);
        die('Could not upload the video correctly.');
    }

    if(!move_uploaded_file($_FILES['browseThumbnail']['tmp_name'], $uploadThumbnailDir))
    {
        removeTreeDir($videoDir);
        die('Could not upload the thumbnail correctly.');
    }

    $con = connectDb();
    $videoQuery = "INSERT INTO video(userId, title, description, foldername, vidFileName, thumbFileName) VALUES ($id,'$title','$description','$folderName', '$vidFileName', '$thumbFileName')";

    if(!mysql_query($videoQuery))
    {
        removeTreeDir($videoDir);
        die('Could not add the video to the database.');
    }

    mysql_close($con);

    echo "true";
?>

Somewhere in the middle of my code I also need a bunch of variables. How would one clean that up? Create an object and aggregate them together?

share|improve this question

1 Answer 1

up vote 1 down vote accepted

Your first if can be simplifed with any function.

function any ($list, $pred) 
{
     foreach ($list as $value) 
     {
          if (call_user_func ($pred, $value) == TRUE)
             return TRUE;
     }
     return FALSE;
}

So it will be :

 $toTest = [$_FILES['browseMovie'], 
            $_FILES['browseThumbnail'], 
            $_GET['title'], 
            $_GET['description'] ]
 if (any ($toTest, "empty")) die ("Please insert all fields")

Then you can simplify the code even more by creating a validate type function

function validateFile ($filePartsMovie, $filePartsThumbnail) 
{
      if($filePartsMovie['extension'] !== "mp4")
      {
           die('Video has to be a .mp4 file.');
      }

      if($filePartsThumbnail['extension'] !== "jpg" &&
         $filePartsThumbnail['extension'] !== "png")
      {
          die('Thumbnail has to be in jpg/jpeg or png format.');
      } 

     if($_FILES['browseMovie']['size'] + $_FILES['browseThumbnail']['size'] > 1600600000)
     {
              die('filesize of video and thumbnail = higher 1.5 GB.');
     }
}

Then you can create a validate function for the rest of the code.

TLDR is to split your code in more separate functions.

BTW I think you do not need brackets when having only one statement after if.

share|improve this answer
    
I do not know PHP so this is from my general programming knowledge. It might not work, but the idea behind the code I posted is good (I hope :)) –  Aleksandar Jan 5 '13 at 11:05
    
Doesnt matter really :) This can happen in any language. It just happened to be written in php when I came across this problem. –  user1534664 Jan 5 '13 at 11:53
    
BTW, I like your validateFile() function. That would definitely make it more organized. :) I dont know about the top one, though. I only need to check if they're empty once. Is that worth the effort adding extra code? –  user1534664 Jan 5 '13 at 11:59
    
"BTW I think you do not need brackets when having only one statement after if." -- You don't, but I nearly always use them for readability. –  s_ha_dum Jan 5 '13 at 15:56
    
Indeed, braceless syntax is allowed, but is usually frowned upon. Additionally that short array syntax ([] instead of array()) is relatively new to PHP (5.4) and may be confusing if someone is unfamiliar with it and attempts to use it with an older version. –  mseancole Jan 7 '13 at 17:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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