Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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

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'))
        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. – Lamonde Feb 26 '13 at 17:53
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? – Lamonde Feb 27 '13 at 13:46

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>";
        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"];
    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
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

Your Answer


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.