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?

UPDATE:

Heres how the code looks now. Its a big improvement compared to its original code. I also modified some things myself. I still think it can be optimized, though:

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

    // ensure all parameters aren't empty
    validateParameters();

    // $fp = fileparts
    $fpMovie = pathinfo($_FILES['browseMovie']['name']);
    $fpThumb = pathinfo($_FILES['browseThumbnail']['name']);

    // ensures the files have the right extensions and are less than 1.5 GB
    validateFiles($fpMovie, $fpThumb);

    $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 = $_FILES['browseThumbnail']['name'];
    $vidFileName = $_FILES['browseMovie']['name'];

    /* every user has its own directory which corresponds to
     * their own username */
    $userDir = "../users/$name/";

    // every user folder contains a videos folder
    $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) will also remove the thumbnail
         * because the thumbnail is in the same folder as the video itsself */
        removeTreeDir($videoDir);
        die('Could not upload the thumbnail correctly.');
    }

    // connectDb() will die() if it can't connect
    $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) will also remove the thumbnail
         * because the thumbnail is in the same folder as the video itsself */
        removeTreeDir($videoDir);
        die('Could not add the video to the database.');
    }

    mysql_close($con);

    echo "true";
?>
share|improve this question
add comment

2 Answers

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
add comment

you use to many die.. But I think my suggestion you collect all error then show in bottom the error not die in every place.. Actualy for query, I think you still can use die(); this is what I fixed for you http://codepaste.net/y2e4pe (see here since the code too long)

share|improve this answer
    
Thanks for your time, mate :) The die() functionality is essential in my program. But why go through all the effort of saving them as an array and looping them afterwards? Wouldn't it be redundant as you could just make the program quit whenever the error occurs and display an error message? Wouldn't that be more readable as well? $_FILES[name][type] is the MIME type by the way. I needed to do a check on the extension hence why I used pathinfo(); –  user1534664 Jan 5 '13 at 12:13
    
The reason I used the get method for the title & the description parameters is because I'm sending a XMLHttpRequest. It's easier in XHR to use GET than POST and theres no point regarding security (as far as I know of). –  user1534664 Jan 5 '13 at 12:16
    
@user1534664: The advice about avoiding die is good. Even with queries. It is a very inelegant way of terminating execution. Using a return in this instance seems best. If it is an error and the execution should be terminated then you should handle that by logging the error and/or loading an error page. Just canceling execution in this way can lead to a blank page with just the die text, which is very ugly. I didn't read the codepaste (its against FAQ for code to be linked to like that), so I can't speak to the rest. –  mseancole Jan 7 '13 at 17:27
    
@mseancole I wont get a blank page. I'm using ajax (xmlhttprequest) to run that code and the output of the die's would be the error I'd have to display. It seems the most functional to be using die because I wouldn't want the rest to run. How would you do it otherwise? –  user1534664 Jan 8 '13 at 11:13
    
@user1534664: Well, that makes more sense then. The die()s are probably fine since they are being handled properly. –  mseancole Jan 8 '13 at 14:27
add comment

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.