current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 3 down vote favorite

My objective is to update necessary data using one function instead of having different functions to update different fields. So, I've created one and I think this is not really elegant, efficient or secure. I would seriously like a review if possible.

Key.

    $data is an array.
    $data[0] = column to update.
    $data[1] = new data.
    $data[2] = username of the user.

Code:

public function handleUserDataUpdate($data) {
    if($userMapper->validate($data) === true) {
        $userMapper->update($data);
    } else {
        $errors['count'] = count($errors);
        return $errors;
    }
}

UserMapper Class Methods:

public function validate($data) {
    switch ($data[0]) {
        case 'rank':
            if(empty($data[1])) {
                $errors[] = "The rank field cannot be empty!";
            }
            if(count($errors) > 0) {
                return $errors;
            } else { 
                return true;
            }
            break;
        case 'display_name':
            if(empty($data[1]) || strlen($data[1]) < 3 || strlen($data[1] > 20)) {
                $errors[] = "The display name should contain at least 3 to 20 characters";
            }
            if(count($errors) > 0) {
                return $errors;
            } else {
                return true;
            }
            break;
    }
}
public function update($data) {
    $sql = "UPDATE users SET " . $data[0] . "=? WHERE username=?";
    $query = $this->db->prepare($this->sql);
    $query->bind_param('ss', $data[1], $data[2]);
    $query->execute();
    $query->close();
}
share|improve this question
    
i would use ? for data[0] too –  Marco Acierno Jun 28 at 12:23
    
@MarcoAcierno what do you mean? –  Hassan Althaf Jun 28 at 12:23
    
Here $sql = "UPDATE users SET " . $data[0] . "=? WHERE username=?"; instead of write " . $data[0] . " you could use ?. –  Marco Acierno Jun 28 at 12:24
1  
I agree Marco, this should work $sql = "UPDATE users SET ?=? WHERE username=?"; $query->bind_param('sss', $data[0], $data[1], $data[2]); –  CodeX Jun 28 at 12:26
    
You use an if-statement to check for errors, then use a second to check if errors have been set. You could compress this to one if/else-statement checking for errors and immediatly returning either the error or (else) returning true. –  GroundZero Jun 28 at 21:47
show 1 more comment

2 Answers

up vote 5 down vote accepted

  1. Why update is a public function? I suppose it's inside a class, so it should be private. If you make it public all checks you do inside handleUserDataUpdate and validate are useless since someone could just use update($data) and put malicious data inside it. The same should be for validate, seems like the only function which should be public is handleUserDataUpdate. And you can be sure, as user of your API i'm more invited to use update than handleUserDataUpdate.

  2. The name handleUserDataUpdate is a bit strange for a public function, i mean handleUserDataUpdate it could be OK for a private method since you can change it every time you want and you free to do what you want. But when it's a public method choose your names careful! updateUserData($data)? Or simply, if the class name is UserDataEditor you can use apply or commit() (If you know Android it could be something like SharedPreferences)

  3. An array? Really, no! Why not a class? I mean, with an array you should remember:

    1. Index 0 is X
    2. Index 1 is Y
    3. Index 2 is Z

    No! Really, after a bit of time you will need to read the source to remember what was 0, 1 and 2.

    You can use a custom class like:

    class UpdateField {
   private $fieldName;
   private $newValue;
   private $userName;
}

    I have the feeling like it could be a great system if:

    1. UpdateField was an interface, which RankUpdate, DisplayNameUpdate implement so inside the class you have the logic of the validate and the ValidatorSystem just execute the method UpdateField->update() which returns an array of errors or true in case of success.

    2. With this you could reuse UpdateField to update everything not just a userfield.

    but instead of use an array, you can improve it:

    1. You can check if fieldName is valid in the constructor of UpdateField and throw an exception if it's not a valid field.
    2. The method validate could be inside this class too, no?

  4. This block of code

    if(count($errors > 0)) {
        return $errors;
} else {
        return true;
}

    ignoring the fact that i hate when a function first return an array or a boolean (i know, it's in the default PHP functions BUT IT'S STUPID! It let me feel like: You will never know what this function will return!) It could be improved in one line with:

    return count($errors) > 0 ? $errors : true;

    but wait, is if(count($errors > 0)) correct? It count the value of a boolean? It's what you want?

  5. @Vogel612 already said about problems inside update functions.

About point 3, take for the moment only the fact that you should use a class instead of an array, about the system i'm thinking it's just an idea in my mind maybe it cannot be done in PHP.

share|improve this answer
    
Love you marco! I want to understand interfaces. Why are they used? If you teach me that I will love you forever!!! –  Hassan Althaf Jun 28 at 12:46
    
Fixed, anyway maybe an interface could not be appropriate here... maybe an abstract class to reduce some duplicate code. If you want to learn interfaces i think it's better to you to read interface page of php. It's nothing of special. –  Marco Acierno Jun 28 at 12:56
    
wtf, its a mistake. I'll fix my main code. –  Hassan Althaf Jun 28 at 13:01
add comment
up vote 4 down vote

You are vulnerable to SQL Injections. Even when you are using Prepared statements, doing It wrong will kill you.

$sql = "UPDATE users SET " . $data[0] . "=? WHERE username=?";

This makes you exposed to SQL injections because your function is not private. You should not allow "user" access to "critical" functions. make that function private and you should be good to go.

As soon as the user is allowed to set $data[0] you're screwed.

share|improve this answer
    
I stated it to you personally that the user is not allowed to set it –  Hassan Althaf Jun 28 at 12:31
    
Not allowed how? By telling them to pretty please not use a public API? –  PeeHaa Jun 28 at 12:32
1  
That's not how it works, public vs private has nothing to do with the SQL injection he has. –  Madara Uchiha Jun 28 at 12:40
1  
@MadaraUchiha in fact it does. If you can ensure that there is no SQL-Critical stuff in $data[0] it should be fine. But as the function is public, you can't ensure that and are thus vulnerable to sql injection –  Vogel612 Jun 28 at 12:52
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.