Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

More than likely, the query isn't built correctly as I am fairly green with SQL.

<?
        $i=0;
        foreach($AddedProducts as $AddedProduct)

        {
            $i++;
            echo "<hr>" . $i . "<hr>";
            $query='INSERT INTO products VALUES(';
            foreach($AddedProduct as $key=>$value)
            {
                $query.=$value. (($key=='Cutsheet_Name')? ') ':', ');
                echo $key . ' = ' . $value . (($key=='Picture_Name')? '.jpg<br>':'<br>');
            }
            $query .= 'ON DUPLICATE KEY UPDATE';
        }
        $statement = $db->prepare($query);
        $statement->execute($query);
?>

Edit: Corrected Version in Progress

    <?
        $i=0;
        foreach($AddedProducts as $AddedProduct)
        {
            $arr=Array();
            $i++;
            echo "<hr>" . $i . "<hr>";
            foreach($AddedProduct as $key=>$value)
            {
                $arr[]=$value;
                echo $key . ' = ' . $value . '<br>';
            }
            $query='INSERT INTO products VALUES(' . implode(', ', $arr) . ')';
            $query .= ' ON DUPLICATE KEY UPDATE ';
            foreach($AddedProduct as $key=>$value)
            {
                $query .= $key . '=' . $value . (($key !='Cutsheet_Name')? ', ':';');
            }
            $statement = $db->prepare($query);
            $statement->execute($query);
        }
    ?>
share|improve this question
Anyone got any further help on how ON DUPLICATE KEY UPDATE should work? What's the easiest way to have it add 26 values all at once to make new rows? – Garet Claborn Jun 29 '11 at 14:16

1 Answer

up vote 1 down vote

It might be just me, but it appears this code could use prepared statements to make it safer. Instead of directly appending values to your INSERT, send in a ? and put what's going to replace it in an array.

In addition to that, it looks like you hard-code what looks like the last value of your array so you can build a valid SQL statement. Instead, you should implode(', ', $an_array).

Also, it is generally considered a bad practice to not specify which fields you're actually referencing in your insert. If someone decides to add in an optional 'Comment' field in your products table then this code is going to break unnecessarily.

It's also worth noting that the ON DUPLICATE KEY UPDATE part looks like its missing what it needs to actually update if there is a duplicate.

Finally, you overwrite $query for every added product in your loop, then only execute the last one. This probably isn't what you intended to do.

share|improve this answer
Oh man, what a mess lol, thank you. What should ON DUPLICATE KEY UPDATE look like? As to adding new fields, this particular part of the app wont ever be exposed to users aside from myself and uses a static format. Even then I just need to add a new property to the class in order to update interaction with table structure. – Garet Claborn Jun 28 '11 at 20:56
I'm trying to add a new row for any new products which each have a unique key Model_Number but update the row if product is already in the table. – Garet Claborn Jun 28 '11 at 21:02

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.