SitePoint Sponsor

User Tag List

Results 1 to 3 of 3
  1. #1
    SitePoint Enthusiast
    Join Date
    Jun 2011
    Posts
    74
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Best practice to build an sql statement for ajax request

    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

    PHP 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. #2
    Programming Since 1978 silver trophybronze trophy felgall's Avatar
    Join Date
    Sep 2005
    Location
    Sydney, NSW, Australia
    Posts
    16,820
    Mentioned
    25 Post(s)
    Tagged
    1 Thread(s)
    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.
    Stephen J Chapman

    javascriptexample.net, Book Reviews, follow me on Twitter
    HTML Help, CSS Help, JavaScript Help, PHP/mySQL Help, blog
    <input name="html5" type="text" required pattern="^$">

  3. #3
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    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.

    PHP Code:
    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?

    PHP Code:
      // 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?

    PHP Code:

          
    }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.

    PHP Code:
        // 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:
    PHP Code:
            $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).


Tags for this Thread

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •