3
\$\begingroup\$

I can't get a peer review where I work, I was wondering how my code was and how could it be improved.

https://github.com/lamondea/uploader

Is it a good practice to use functions in classes or should it be replaced by external function that use the object returned by the class:

class _export
{   
    public $nbr;
    public $file;

    public function __construct($file,$nbr=0){
        if($nbr != 0 && ($file->ext == 'jpg' || $file->ext == 'png' || $file->ext == 'gif'))
            thumbnailer($file,$nbr);
        else {
            move_uploaded_file($file->tmp_name, ($file->loc).($file->name));
            return $file->name;
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ I really never used peer review and I just started using classes, so I guess the whole /class/uploader.class.php would be the important for that matter. I will add a part, but still it would be hard to choose a part since the uploader work. \$\endgroup\$
    – Lamonde
    Feb 26, 2013 at 17:53
  • 2
    \$\begingroup\$ Two general things about the code, 1) class name shouldn't start with "_", 2) Looks like you are doing the validation in constructor which is not good. Let the constructor just for initialization stuff. \$\endgroup\$
    – Kinjal
    Feb 27, 2013 at 8:36
  • \$\begingroup\$ Is there a norm about naming the classes? \$\endgroup\$
    – Lamonde
    Feb 27, 2013 at 13:46

1 Answer 1

2
\$\begingroup\$

I'm not sure if using classes is better or worse but I am starting to see files being called as functions more than a single file with function within. This is a more common upload script.

$allowedExts = array("gif", "jpeg", "jpg", "png");
$temp = explode(".", $_FILES["file"]["name"]);
$extension = end($temp);

if ((($_FILES["file"]["type"] == "image/gif")
    || ($_FILES["file"]["type"] == "image/jpeg")
    || ($_FILES["file"]["type"] == "image/jpg")
    || ($_FILES["file"]["type"] == "image/pjpeg")
    || ($_FILES["file"]["type"] == "image/x-png")
    || ($_FILES["file"]["type"] == "image/png"))
    && ($_FILES["file"]["size"] < 20000)
    && in_array($extension, $allowedExts)){
    if ($_FILES["file"]["error"] > 0){
        echo "Error: " . $_FILES["file"]["error"] . "<br>";
    }
    else{
        echo "Upload: " . $_FILES["file"]["name"] . "<br>";
        echo "Type: " . $_FILES["file"]["type"] . "<br>";
        echo "Size: " . ($_FILES["file"]["size"] / 1024) . " kB<br>";
        echo "Stored in: " . $_FILES["file"]["tmp_name"];
    }
}
else{
    echo "Invalid file";
}
\$\endgroup\$
2
  • \$\begingroup\$ The $allowedExts array is a good idea! You could use a similar array for $_FILES["file"]["type"] too. \$\endgroup\$
    – palacsint
    Nov 6, 2013 at 20:53
  • 1
    \$\begingroup\$ Agreed, i would shorten this and say if($_FILES["file"]["type"] is in $ArrayofMetaData) or something to that affect. It would look much cleaner. \$\endgroup\$
    – epool
    Nov 6, 2013 at 21:13

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.