2
\$\begingroup\$

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?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ 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 :)) \$\endgroup\$
    – Aleksandar
    Jan 5, 2013 at 11:05
  • \$\begingroup\$ Doesnt matter really :) This can happen in any language. It just happened to be written in php when I came across this problem. \$\endgroup\$ Jan 5, 2013 at 11:53
  • \$\begingroup\$ 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? \$\endgroup\$ Jan 5, 2013 at 11:59
  • \$\begingroup\$ "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. \$\endgroup\$
    – s_ha_dum
    Jan 5, 2013 at 15:56
  • \$\begingroup\$ 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. \$\endgroup\$
    – mseancole
    Jan 7, 2013 at 17:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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