2
\$\begingroup\$

I've written an image resizer / uploader, that takes an incoming image, resizes it, and uploads it to the server, under the /usericons/ folder.

However, the code is sloppy. Can somebody suggest a better method / improvements?

define ("MAX_SIZE","400");

$errors = 0;

if ($_SERVER["REQUEST_METHOD"] == "POST") {
  $image = $_FILES["icon"]["name"];
  $uploadedfile = $_FILES['icon']['tmp_name'];
  if ($image) {
        $filename = stripslashes($_FILES['icon']['name']);
        $extension = getExtension($filename);
        $extension = strtolower($extension);
        if (($extension != "jpg") && ($extension != "jpeg") && ($extension != "png") && ($extension != "gif")) {
              echo ' Unknown Image extension ';
              $errors = 1;
        } else {
              $size=filesize($_FILES['icon']['tmp_name']);
              if ($size > MAX_SIZE*1024) {
                    echo "You have exceeded the size limit";
                    $errors = 1;
              }
              if ($extension == "jpg" || $extension == "jpeg" ) {
                    $uploadedfile = $_FILES['icon']['tmp_name'];
                    $src = imagecreatefromjpeg($uploadedfile);
              } else if($extension=="png") {
                    $uploadedfile = $_FILES['icon']['tmp_name'];
                    $src = imagecreatefrompng($uploadedfile);
              } else {
                    $src = imagecreatefromgif($uploadedfile);
              }
              list($width,$height)=getimagesize($uploadedfile);
              $newwidth = 500;
              $newheight = ($height/$width)*$newwidth;
              $tmp = imagecreatetruecolor($newwidth,$newheight);
              $newwidth1 = 300;
              $newheight1 = ($height/$width)*$newwidth1;
              $tmp1 = imagecreatetruecolor($newwidth1,$newheight1);
              imagecopyresampled($tmp, $src, 0,0,0,0, $newwidth, $newheight, $width, $height);
              imagecopyresampled($tmp1, $src, 0,0,0,0, $newwidth1, $newheight1, $width, $height);
              $filename = "usericons/500-". $_SESSION['username'] . '.png';
              $filename1 = "usericons/300-". $_SESSION['username'] . '.png';
              imagejpeg($tmp,$filename,100);
              imagejpeg($tmp1,$filename1,100);
              hasIcon($_SESSION['username']);
              imagedestroy($src);
              imagedestroy($tmp);
              imagedestroy($tmp1);
              echo '<script>window.location = "/Users/";</script>';
        }
  }
}
 //If no errors registred, print the success message

if (isset($_POST['Submit']) && !$errors) {
   echo "Image Upload Fail!";
}
function getExtension($str) {

     $i = strrpos($str,".");
     if (!$i) { return ""; } 
     $l = strlen($str) - $i;
     $ext = substr($str,$i+1,$l);
     return $ext;
 }
\$\endgroup\$
2
\$\begingroup\$

Couple of things I've done to your code:

  • Early exits
  • Redundant code removal
  • Readability improvement

Keep in mind that I have not tested this, but it should work. And also, the code will only work on PHP 5.4 and greater.

Cleaned up version:

<?php
if ($_SERVER['REQUEST_METHOD'] !== 'POST' || !isset($_FILES['icon'])) {
    echo 'Image upload failed.';

    return;
}

$file = [
    'name' => stripslashes($_FILES['icon']['name']),
    'path' => $_FILES['icon']['tmp_name'],
    'ext'  => strtolower(pathinfo($_FILES['icon']['name'], PATHINFO_EXTENSION)),
    'size' => filesize($_FILES['icon']['tmp_name'])
];

$supportedExts = [
    'jpg',
    'jpeg',
    'png',
    'gif'
];

if (!in_array($file['ext'], $supportedExts)) {
    echo 'Extension not supported.';

    return;
}

if ($file['size'] > 400 * 1024) {
    echo 'You have exceeded the image size limit.';

    return;
}

list($image['x'], $image['y']) = getimagesize($file['path']);

switch ($file['ext']) {
    case 'jpg':
    case 'jpeg':
        $imgResrc1 = imagecreatefromjpeg($file['path']);
        break;
    case 'png':
        $imgResrc1 = imagecreatefrompng($file['path']);
        break;
    default:
        $imgResrc1 = imagecreatefromgif($file['path']);
}

$imageSizes = array(
    [
        'x' => 300,
        'y' => $image['y'] / $image['x'] * 300
    ],
    [
        'x' => 500,
        'y' => $image['y'] / $image['x'] * 500
    ]
);

foreach ($imageSizes as $size) {
    $imgResrc2 = imagecreatetruecolor($size['x'], $size['y']);

    imagecopyresampled($imgResrc2, $imgResrc1, 0, 0, 0, 0, $size['x'], $size['y'], $image['x'], $image['y']);

    // Shouldn't the extension be .jpg / .jpeg since you're using imagejpeg()?
    $filePath = 'usericons/' . $size['x'] . '-'. $_SESSION['username'] . '.png';

    imagejpeg($imgResrc2, $filePath, 100);

    imagedestroy($imgResrc2);
}

imagedestroy($imgResrc1);

hasIcon($_SESSION['username']);

echo '<script>window.location = \'/Users/\';</script>';

// TODO: Print success message
\$\endgroup\$
1
\$\begingroup\$

function getExtension() replacement

you can replace it with this code

$filename = stripslashes($_FILES['icon']['name']);
$extension = pathinfo($filename, PATHINFO_EXTENSION);

so you can safely remove function getExtension() because you don't need it anymore.

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