This method receives an array of data and does some stuff in a database. I need to get some reviews on it if possible.

public function doSomeStuff($arr = array())
    $id = $arr['Employee']['id'];
    $name = $arr['Employee']['name'];
    $status = $arr['Employee']['status'] == 'Disabled' ? 0 : 1;
    $user_id = $arr['Employee']['user_id'];
    $query = "update `mytable` set `status` = $status, `name`=$name   WHERE `user_id` = ?";
    self::_runthis($query, array($user_id));

I am looking to see if this is fool-proof for the data it will receive and will process it.


1 Answer 1

  • Instead of accessing the employee key every time, just pass in the array rooted at the Employee key
  • What happens if all of the array indexes are not set? You should either check for that or use an object on which you know they all exist
    • Important mostly for data integrity (what if you create an Employee array somewhere and forget a status or something?)
    • But also accessing non-set array keys issues a notice
  • I suspect there's something wrong with your class design as a whole. In particular, the same class should probably not have a method to update an employee and a method to run a query (you don't happen to have something extending some vague DB class do you...?)
  • Why is name not a place holder like the user_id?
  • Use real names when posting code here -- doSomeStuff is very vague, and that hampers our ability to review
  • A text status should probably not be being passed into this since it seems like you're dealing with a low level record here
  • Don't quote entity names unless you need to. It breaks compatibility across SQL-dialects for no reason
  • What if $name has spaces? What if it has a ' in it? Look into SQL injection and prepared statements. (They're not just about security. There also about correctness. In it's current form, your code has a very major bug.)
  • A default value for the parameter shouldn't be provided. Would you want someone to call $obj->doSomeStuff();

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.