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.

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;
        }
    }
}
share|improve this question
    
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. –  Zeppo Feb 26 '13 at 17:53
2  
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. –  Kinjal Feb 27 '13 at 8:36
    
Is there a norm about naming the classes? –  Zeppo Feb 27 '13 at 13:46
add comment

1 Answer

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";
}
share|improve this answer
    
The $allowedExts array is a good idea! You could use a similar array for $_FILES["file"]["type"] too. –  palacsint Nov 6 '13 at 20:53
1  
Agreed, i would shorten this and say if($_FILES["file"]["type"] is in $ArrayofMetaData) or something to that affect. It would look much cleaner. –  epool Nov 6 '13 at 21:13
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.