Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Most important of all, you should not use string concatenation to set values in an SQL statement like this:

$conn->query("INSERT INTO `product` (".$fields.") VALUES (".$values.")");

You should use prepared statements instead. It will make your queries safer as prepared statements check value types and prevent sql injection attacks, and it can make queries more efficient as they can be compiled and only the dynamic parameters inserted.


When you check if a boolean value is true or false, it's better to write like this:

// instead of $valid == true
if ($isValid) {

// instead of $valid == false
if (!$isValid) {

Related to this, at the end of your code you do this:

if($isValid == false){
    // ...
}
if($isValid == true){
    // ...
}

Inside the { ... } blocks you don't change the value of $isValid, so these two blocks are mutually exclusive and should have been written with an if-else like this:

if (!$isValid) {
    // ...
} else {
    // ...
}

This looks strange:

foreach($_POST['gegevens'] as $key => $value){
    if(in_array($key,$verplichtArray)){
        if($value == ""){
            $isValid = false;
            $fouteVelden[] = $key;
            $message = "Een of meerdere van de velden waren leeg.";
        }
    }
}

If there are multiple $value == "", the $message will be overwritten. But of course that's normal if I paste your message into Google Translate, it turns out it means "One or more of the fields were empty" which makes sense :-) So yes as @malachi@malachi it would have helped to translate the non-English texts when you post on an English-language forum like this one ;-)

Also, the two if statements can be combined, and it's good to put a space after comma, after ) and before (:

foreach($_POST['gegevens'] as $key => $value) {
    if (in_array($key, $verplichtArray) && $value == "") {
        $isValid = false;
        $fouteVelden[] = $key;
        $message = "Een of meerdere van de velden waren leeg.";
    }
}

Most important of all, you should not use string concatenation to set values in an SQL statement like this:

$conn->query("INSERT INTO `product` (".$fields.") VALUES (".$values.")");

You should use prepared statements instead. It will make your queries safer as prepared statements check value types and prevent sql injection attacks, and it can make queries more efficient as they can be compiled and only the dynamic parameters inserted.


When you check if a boolean value is true or false, it's better to write like this:

// instead of $valid == true
if ($isValid) {

// instead of $valid == false
if (!$isValid) {

Related to this, at the end of your code you do this:

if($isValid == false){
    // ...
}
if($isValid == true){
    // ...
}

Inside the { ... } blocks you don't change the value of $isValid, so these two blocks are mutually exclusive and should have been written with an if-else like this:

if (!$isValid) {
    // ...
} else {
    // ...
}

This looks strange:

foreach($_POST['gegevens'] as $key => $value){
    if(in_array($key,$verplichtArray)){
        if($value == ""){
            $isValid = false;
            $fouteVelden[] = $key;
            $message = "Een of meerdere van de velden waren leeg.";
        }
    }
}

If there are multiple $value == "", the $message will be overwritten. But of course that's normal if I paste your message into Google Translate, it turns out it means "One or more of the fields were empty" which makes sense :-) So yes as @malachi it would have helped to translate the non-English texts when you post on an English-language forum like this one ;-)

Also, the two if statements can be combined, and it's good to put a space after comma, after ) and before (:

foreach($_POST['gegevens'] as $key => $value) {
    if (in_array($key, $verplichtArray) && $value == "") {
        $isValid = false;
        $fouteVelden[] = $key;
        $message = "Een of meerdere van de velden waren leeg.";
    }
}

Most important of all, you should not use string concatenation to set values in an SQL statement like this:

$conn->query("INSERT INTO `product` (".$fields.") VALUES (".$values.")");

You should use prepared statements instead. It will make your queries safer as prepared statements check value types and prevent sql injection attacks, and it can make queries more efficient as they can be compiled and only the dynamic parameters inserted.


When you check if a boolean value is true or false, it's better to write like this:

// instead of $valid == true
if ($isValid) {

// instead of $valid == false
if (!$isValid) {

Related to this, at the end of your code you do this:

if($isValid == false){
    // ...
}
if($isValid == true){
    // ...
}

Inside the { ... } blocks you don't change the value of $isValid, so these two blocks are mutually exclusive and should have been written with an if-else like this:

if (!$isValid) {
    // ...
} else {
    // ...
}

This looks strange:

foreach($_POST['gegevens'] as $key => $value){
    if(in_array($key,$verplichtArray)){
        if($value == ""){
            $isValid = false;
            $fouteVelden[] = $key;
            $message = "Een of meerdere van de velden waren leeg.";
        }
    }
}

If there are multiple $value == "", the $message will be overwritten. But of course that's normal if I paste your message into Google Translate, it turns out it means "One or more of the fields were empty" which makes sense :-) So yes as @malachi it would have helped to translate the non-English texts when you post on an English-language forum like this one ;-)

Also, the two if statements can be combined, and it's good to put a space after comma, after ) and before (:

foreach($_POST['gegevens'] as $key => $value) {
    if (in_array($key, $verplichtArray) && $value == "") {
        $isValid = false;
        $fouteVelden[] = $key;
        $message = "Een of meerdere van de velden waren leeg.";
    }
}
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

Most important of all, you should not use string concatenation to set values in an SQL statement like this:

$conn->query("INSERT INTO `product` (".$fields.") VALUES (".$values.")");

You should use prepared statements instead. It will make your queries safer as prepared statements check value types and prevent sql injection attacks, and it can make queries more efficient as they can be compiled and only the dynamic parameters inserted.


When you check if a boolean value is true or false, it's better to write like this:

// instead of $valid == true
if ($isValid) {

// instead of $valid == false
if (!$isValid) {

Related to this, at the end of your code you do this:

if($isValid == false){
    // ...
}
if($isValid == true){
    // ...
}

Inside the { ... } blocks you don't change the value of $isValid, so these two blocks are mutually exclusive and should have been written with an if-else like this:

if (!$isValid) {
    // ...
} else {
    // ...
}

This looks strange:

foreach($_POST['gegevens'] as $key => $value){
    if(in_array($key,$verplichtArray)){
        if($value == ""){
            $isValid = false;
            $fouteVelden[] = $key;
            $message = "Een of meerdere van de velden waren leeg.";
        }
    }
}

If there are multiple $value == "", the $message will be overwritten. But of course that's normal if I paste your message into Google Translate, it turns out it means "One or more of the fields were empty" which makes sense :-) So yes as @malachi it would have helped to translate the non-English texts when you post on an English-language forum like this one ;-)

Also, the two if statements can be combined, and it's good to put a space after comma, after ) and before (:

foreach($_POST['gegevens'] as $key => $value) {
    if (in_array($key, $verplichtArray) && $value == "") {
        $isValid = false;
        $fouteVelden[] = $key;
        $message = "Een of meerdere van de velden waren leeg.";
    }
}