Best practice to build an sql statement for ajax request

PHP
1

I have built an ajax page that gets data by checking which value is selected from the dropdown box. While the code below works perfectly for all possible selections it requires more code for more choices. Is there a better practice available to use more efficient code

I have built an ajax page that gets data by checking which value is selected from the dropdown box. While the code below works perfectly for all possible selections it requires more code for more choices. Is there a better practice available to use more efficient code

$sql="SELECT m.id memberId, m.firstName firstName, m.surName surName, m.grade grade, m.dueDate dueDate, m.dob dob, m.email email, m.phone phone, c.name clubName FROM members AS m
      LEFT JOIN clubs AS c ON m.club = c.id";
if(isset($_GET)){
  // get data
  $g=$_GET["g"];
  $u=$_GET["u"];
  $d=$_GET["d"];

  // Check is grade is set
  if($g == 0){
    // if g is not set
    // check if u is set
    if($u != "0"){
      $sql .= " WHERE c.name = '$u'";
      if($d == 1){
        $today = date('Y-m-d');
        $sql .= " AND dueDate < '$today'";
      }
    }else{
      if($d == 1){
        $today = date('Y-m-d');
        $sql .= " WHERE dueDate < '$today'";
      }
    }
  }else{
    // if g is set
    $sql.= " WHERE grade = '".$g."'";
    if($d == 1){
        $today = date('Y-m-d');
        $sql .= " AND dueDate < '$today'";
      }
    // check is u is set
    if($u != "0"){
      $sql .= " AND c.name = '$u'";
      if($d == 1){
        $today = date('Y-m-d');
        $sql .= " AND dueDate < '$today'";
      }
    }
  }
}
2

Best practice would be to PREPARE the query and pass the variables to use with it separately then use BIND to attach the data to the prepared sql. Jumbling SQL and data in the same statement is not a good idea (it makes injection possible) and by using PREPARE/BIND you can keep them separate.

That would mean rewriting the PHP to use PREPARE and BIND and then simply substituting the values passed to it into the BIND statement.

3

I think there are lots of bits of problems with the code you have written.

I’ll try and pick them out, and you can reply if you want to.


if(isset($_GET)){ 
  // get data 
  $g=$_GET["g"]; 
  $u=$_GET["u"]; 
  $d=$_GET["d"];

Above, your script assumes that if any GET var is set, then $g, $u and $d are also set, if one is missing you are going to have errors.

You make no attempt to filter these variables as they come in.

Are they all integers? Is there a permitted range for these numbers? Can they be negative?

Are they strings? Do you have a list of permitted strings?

What should happen if one is missing, is there a default value you’d like to set, or does a missing value mean someone is fiddling with your form and you’d prefer to abort?


  // Check is grade is set  
  if($g == 0){ 
    // if g is not set 
    // check if u is set 
    if($u != "0"){ 
      $sql .= " WHERE c.name = '$u'"; 
      if($d == 1){ 
        $today = date('Y-m-d'); 
        $sql .= " AND dueDate < '$today'"; 
      }

So, if $d is not zero am I correct is assuming that you will be adding " AND dueDate < ‘$today’"; to every query, no matter what the status of $c or $g ?

Why is the check for $g == 0 (a loose check for zero, can be false, can be empty string) and the check for $u is not equal to a string containing 0.

Use === for conditional checks, if it should be a string containing 0 then use === “0”

You’ve now set a variable called $today in 4 different places, why not just set that at the top?



      }else{ 
      if($d == 1){ 
        $today = date('Y-m-d'); 
        $sql .= " WHERE dueDate < '$today'"; 
      } 
    } 
  }else{ 
    // if g is set 
    $sql.= " WHERE grade = '".$g."'"; 
    if($d == 1){ 
        $today = date('Y-m-d'); 
        $sql .= " AND dueDate < '$today'"; 
      }

Above, your quoting is inconsistent here, you will end up tripping yourself up.

Here you concatenate the variable: $sql.= " WHERE grade = ‘“.$g.”’“;
Here you expand the variable without concatenation: $sql .= " AND dueDate < ‘$today’”;

They are both correct but choose one or the other and stick to it.


    // check is u is set 
    if($u != "0"){ 
      $sql .= " AND c.name = '$u'"; 
      if($d == 1){ 
        $today = date('Y-m-d'); 
        $sql .= " AND dueDate < '$today'"; 
      } 
    } 
  } 
}

Your query is the equivalent of

“select this, that from my table WHERE date > today”

What you have not left room for is other SQL instructions which could come after the WHERE clause.

What you have is a section of WHERE clauses, so I’d call them that:


        $where = " AND dueDate < '$today'";

This leaves the door open to do this after all the data is collected.

"select this, that from my table " . $where . $group_by . $limit ;

… rather that keep concatenating to the sql statement as you go.

I had difficulty following all the ifs, so tell me is any of this true:

  1. $g can be set, but not $u
  2. $u can be set, but not $g
  3. Both $g and $u MUST be set
  4. Both $g and $u can be NOT set
  5. $d can be set or unset no matter what $g and $u contain.

Overall though your question is a good one, once you get into multiple nested if/else conditions you should be asking yourself that exact question.

I can think of some improvements, but I don’t want to assume too much till you reply.

Refactoring out if conditions can be done, but I think in your case you could improve that code quite simply.

Are you able to show another variable that you could perhaps be introducing? This might prompt a reply from others too.

I’ve paved the way for the introduction of pagination with mention of a $limit clause (“show next 10” buttons and the like).

As Felgall says, you should really be using prepared statements now, and that would be the ultimate challenge – make the code more readable, obvious, up to date, robust and secure.

EDIT
You asked the question twice at the top.

I have built an ajax page that gets data by checking which value is selected from the dropdown box. While the code below works perfectly for all possible selections it requires more code for more choices. Is there a better practice available to use more efficient code

I have built an ajax page that gets data by checking which value is selected from the dropdown box. While the code below works perfectly for all possible selections it requires more code for more choices. Is there a better practice available to use more efficient code

I find this most amusing since one of the code errors I see that I would like to address is the idea of DRY (Don’t Repeat Yourself). :slight_smile: