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.
public function __i($table, $arr) { $c=$this->connection;
   foreach ( $arr as $name => $val):
      $param1.=$name . ",";
      $param2.="?,";
   endforeach;

   $PARAM1 = substr_replace($param1, '', -1);
   $PARAM2 = substr_replace($param2, '', -1);

   $query = 'INSERT INTO '.$table.' ('.$PARAM1.') VALUES ('.$PARAM2.')';

   echo $query;
   foreach ( $arr as $name => $val):
    if ( is_int($val)) :
         $param = 'i';
      endif;

      if ( is_string($val)) :
         $param = 's';
      endif;

      if ( is_double($val)) :
         $param = 'd';
      endif;
      $binds.=$param;
      $values.="'".$arr[$name]."',";

    endforeach;

    $values = substr_replace($values, '', -1);
    echo "BINDS ARE: ".$binds . "<br>";
    echo "VALUES ARE: ".$values."<br>";

   if ( $s = $c->prepare($query)):

      $args = $binds + $values;
      call_user_func_array(array($s, 'bind_param'), array($binds, $values));
      $s->execute();
      $s->close();
   endif;

}
share|improve this question
add comment

1 Answer 1

Well, there's no context, so I can't be sure of exactly what you want, so I will assume a general review. In the future, please provide a little intro, even if its only to say, "please review and tell me how I can do better." Just dumping code is rather vague, for instance, this question could just as easily been about a better way to insert, of which I have no idea.

First, your method and variable names could use some work. What is __i()? A little more descriptive please. I can assume, based on the title of this post that this is an insert() method, but without any context this would be very difficult to determine. Additionally, I would avoid using double-spaced method names, those are usually reserved for PHP magic methods, such as __construct(). A single space is good enough, though that usually refers to a private or protected method.

Start your method's body on a newline, rather that the same one that the method was started on. I don't know if this was just a formatting issue with SO, or if its actually like that in your code. And, as above, what is $c? Connection can be inferred, but anyone reading this should not have to guess. $conn is a typical shortening and is widely accepted. Or you could spell it out completely, or use the root word "connect". As long as its somehow logically connected with the intent.

public function __i($table, $arr) {
    $conn = $this->connection;

Why are you using the standard template syntax for your statements and loops? In templates (Views or HTML files with PHP in them) this is fine, but it is quite odd and a little difficult to read in normal class structures. The preferred method is to use normal syntax, like so:

if() {
}
//AND
foreach() {
}

Where did $param1 and $param2 come from? They were not passed in as parameters to the method, which makes their names a little confusing. I would assume these are either globals, which are bad, or just undefined variables, which are also bad. If the later, then on the first iteration of this foreach loop those two variables will produce warnings about not being defined, or, god forbid, should there be globals assigned with those names, then you might just start appending on to them. To prevent this kind of mistake, it is best to define them before trying to use them. Also, any variable with a number after it is typically frowned upon. Use that variable's intent to better name them.

$param1 = '';
//etc...
$param1 .= $name . ",";

I know what these two substr_replace() lines are for, but they might be a little confusing to others. Perhaps a better method would be to use rtrim() to explicitly remove the last comma. This will make it more obvious what you are trying to do, and slightly decrease the size of that line. I don't know if its any faster or slower, but I don't think the difference will matter much here.

$PARAM1 = rtrim( $param1, ',' );

Your $param is not always set. This means that should $val not be an int, string, or double, then it will use the last known value of $param from the previous iteration. This can result in unforeseen results. Instead, you should either set up a default value or use an if/elseif/else structure. Here's a simple example assuming string as a default, you can always extend else in the following example to another elseif and use a final else to throw an error if you don't want to default.

if( is_int( $val ) ) {
    $param = 'i';
} elseif( is_double( $val ) ) {
    $param = 'd';
} else {
    $param = 's';
}
//which can easily be rewritten to use a default as
$param = 's';
if( is_int( $val ) {
    $param = 'i';
} elseif( is_double( $val ) ) {
    $param = 'd';
}

Don't define a variable in an if statement. It is too easy to accidentally do so when you don't mean to and will not always be obvious that you did not mean to perform a comparison instead.

$s = $c->prepare( $query ):
if( $s ) {
    //etc...
}

I'm not really sure how I feel about the call_user_func_array(), It could be a little confusing to someone who's never seen it before, but that's your call.

Finally, you might want to break up this method into multiple smaller methods. Use the Single Responsibility and "Don't Repeat Yourself" (DRY) Principles as a guideline. For example, to help you get started you can move that first foreach loop and those two substr_replace() lines into a new method called getParams() or something more aptly named. This will make it easier to reuse this code should you ever need to and make it easier to read.

share|improve this answer
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.