5
\$\begingroup\$

I have a website, where one of the options that a user gets is to change their avatar.

I would like to know if there is anything that I shouldn't or should be doing... So here is what I've got.

Here is my PHP file that handles uploads:

<?php include('./config.php');
    $user = $_SESSION['user']['id'];
    $hash = randHash();

    $tempUser = './assets/temp/faces/' .$user. '/';
    $tempUpload = $tempUser . $hash. '/';

    while(is_dir($tempUpload)) {
        $hash = randHash();
        $tempUpload = $tempUser . $hash. '/';
    }

    mkdir($tempUser);
    mkdir($tempUpload);

    $tempFile = $tempUpload . basename($_FILES['avatar']['name']);

    $fileType = pathinfo($tempFile, PATHINFO_EXTENSION);
    $fileName = $hash;
    $fileName .= '.' .$fileType;

    if($_FILES['avatar']['name']) {
        if(
        $_FILES['avatar']['type'] == 'image/jpeg'
        || $_FILES['avatar']['type'] == 'image/jpg'
        || $_FILES['avatar']['type'] == 'image/png'
        || $_FILES['avatar']['type'] == 'image/gif'
        ) {
            if(move_uploaded_file($_FILES['avatar']['tmp_name'], $tempFile)) {
                rename($tempFile, $tempUpload . $fileName);
                $tempFile = $tempUpload . $fileName;

                list($width, $height) = getimagesize($tempFile);
                if($width > 400 || $height > 400) {
                    unlink($tempFile);
                    rmdir($tempUpload);
                    rmdir($tempUser);
                    echo 'error:004';
                } else {
                    if(!is_dir('./assets/faces/' .$user. '/')) {
                        mkdir('./assets/faces/' .$user. '/');
                    };
                    rename($tempFile, './assets/faces/' .$user. '/' .$fileName);
                    rmdir($tempUpload);
                    rmdir($tempUser);
                    $file = './assets/faces/' .$user. '/' .$fileName;
                    echo $file;

                    $get = $users->prepare("SELECT userAvatar FROM userDetails WHERE userID=?");
                    $get->bind_param('i', $user);
                    $get->execute();
                    $get->bind_result($oldFile);
                    $get->fetch();
                    $get->close();

                    if($oldFile != "./assets/faces/default-face.png") { unlink($oldFile); }

                    $put = $users->prepare("UPDATE userDetails SET userAvatar=? WHERE userID=?");
                    $put->bind_param('si', $file,$user);
                    $put->execute();
                    $put->close();
                }
            } else {
                rmdir($tempUpload);
                echo 'error:003';
            };
        } else {
            rmdir($tempUpload);
            echo 'error:002';
        };
    } else {
        rmdir($tempUpload);
        echo 'error:001';
    };
?>

Here is the HTML/Ajax that handles the form:

<div class="grp-con mrg-top-med">
    <p class="title">Avatar</p>
    <ul>
        <li class="mrg-rgt-med"><img class="face-preview-lrg" src="<?php echo $userFace; ?>" /></li>
        <li>
            <script>
                $(document).ready(function() {
                    $('#avatar').on('submit', function(e) {
                        var form = $(this);
                        var avatar = form.children('input[type=file]');
                        var btn = form.children('button[type=submit]');
                        var fd = new FormData();
                        fd.append('avatar', avatar[0].files[0]);

                        btn.prepend('<i class="fa fa-fw fa-refresh fa-spin"></i> ');

                        $.ajax({
                            url:'./user-update-avatar.php',
                            data:fd,
                            processData:false,
                            contentType:false,
                            type:'POST'
                        })
                        .done(function(data) {
                            if(data.indexOf('error') != -1) {
                                btn
                                    .removeAttr('class')
                                    .addClass('btn-danger mrg-top-med')
                                    .html('<i class="fa fa-fw fa-close"></i>');
                            } else {
                                btn
                                    .removeAttr('class')
                                    .addClass('btn-success mrg-top-med')
                                    .html('<i class="fa fa-fw fa-check"></i>');
                                $('.face-preview-lrg').attr('src', data);
                            };
                            window.setTimeout(function() {
                                btn
                                    .removeAttr('class')
                                    .addClass('btn-seagreen mrg-top-med')
                                    .html('Change Avatar');
                            }, 2000);
                        })
                        .fail(function(data) {
                            btn
                                .removeAttr('class')
                                .addClass('btn-danger mrg-top-med')
                                .html('<i class="fa fa-fw fa-close"></i>');
                            window.setTimeout(function() {
                                btn
                                    .removeAttr('class')
                                    .addClass('btn-seagreen mrg-top-med')
                                    .html('Change Avatar');
                            }, 2000);
                        });

                        e.preventDefault();
                    });
                })
            </script>
            <form id="avatar" method="post" enctype="multipart/form-data">
                <p>No larger than 400x400</p>
                <input name="avatar" type="file" required />
                <button class="btn-seagreen mrg-top-med" type="submit">Change Avatar</button>
            </form>
        </li>
    </ul>
</div>

What are your thoughts?

\$\endgroup\$
4
\$\begingroup\$

Return/Exit early

When validating things, it's always good to handle the exceptional cases first. With your current code, it's hard to see what actually happens in the else case, or the other way around: it's hard to see which else closes which if.

You could reduce the level of nesting you have, and increase readability, by writing it like this:

if(!$_FILES['avatar']['name']) { 
    rmdir($tempUpload);
    echo 'error:001';
    // return/exit
}

if($_FILES['avatar']['type'] !== 'image/jpeg'
    && $_FILES['avatar']['type'] !== 'image/jpg'
    && $_FILES['avatar']['type'] !== 'image/png'
    && $_FILES['avatar']['type'] !== 'image/gif'
    ) {
    rmdir($tempUpload);
    echo 'error:002';
    // return/exit
}

// upload

Separating code into blocks

Your code does a lot in one block. It uploads an image, it validates data, saves data in the database, prints data, etc. This makes your code hard to read, hard to maintain, hard to test, and hard to reuse. Think about creating different methods and classes for the different functionality.

Handling Directories and moving files

You have quite a lot of code handling directories. Why are you creating a temporary directory when you don't even know yet if you can upload to it, just to remove it again? It would be easier to just create it once you know that it's actually needed.

You also move the uploaded file around a lot. It first goes to $tempFile, then to $tempUpload . $fileName, until it finally is saved in the actual directory. The first move seems unnecessary and is also extremely confusing (you move tempFile, and then change the $tempfile variable to the new location), and the second move should also be easy to avoid by first retrieving the old file.

\$\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.