0

I have 4 if/else conditions, Out of 4 only 3 work fine, The values in the condition would have a combination of numeric and text, is this causing the issue.

I am not sure, can some one tell me if this is causing the issue, is there any other way of doing this.

In the below code I have mentioned the values for 2 variables passed from a form. In this scenario ideally it should go to 3rd If condition, but it goes to 4th if condition.

part of my php code:

    echo "category :".$option." ".$suboption." "; //Values displayed for $option is 4 and $suboption is Nitrogen

    if ($option==0 && $suboption==0)
        $dc=mysql_query("SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER ORDER BY Ac_code, Prod_desc");
    else{
        if($option==0 && $suboption!=0)
            $dc=mysql_query("SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER WHERE Prod_desc='$suboption' ORDER BY Ac_code, Prod_desc");
        else{
           if($option!=0 && $suboption!=0)
               $dc=mysql_query("SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER WHERE Ac_code='$option' AND Prod_desc='$suboption' ORDER BY Ac_code, Prod_desc");
           else{
               if($option!=0 && $suboption==0)
                  $dc=mysql_query("SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER WHERE Ac_code='$option' ORDER BY Ac_code, Prod_desc");
           }
        }
    }
Bhavik Shah
  • 2,300
  • 1
  • 17
  • 32
user1114409
  • 565
  • 3
  • 13
  • 26
  • Oh man write `else if` or `elseif` on one line or use two nested ifs in your case – Alvin Wong Jan 11 '13 at 06:20
  • 1
    you should think about using an other option. using switch seems more clear in that case? – devanand Jan 11 '13 at 06:21
  • 1
    Perhaps switch is a better option for this case.... No pun intended. – sbeliv01 Jan 11 '13 at 06:22
  • [**Please, don't use `mysql_*` functions in new code**](http://bit.ly/phpmsql). They are no longer maintained [and are officially deprecated](https://wiki.php.net/rfc/mysql_deprecation). See the [**red box**](http://j.mp/Te9zIL)? Learn about [*prepared statements*](http://j.mp/T9hLWi) instead, and use [PDO](http://php.net/pdo) or [MySQLi](http://php.net/mysqli) - [this article](http://j.mp/QEx8IB) will help you decide which. If you choose PDO, [here is a good tutorial](http://stackoverflow.com/a/14110189/1723893). – NullPoiиteя Jan 11 '13 at 06:34
  • @sbeliv01 I tried both switch and elseif, it is still not working. – user1114409 Jan 11 '13 at 06:36

4 Answers4

1

This is bad code.

  1. Do not use so many if else conditions, define variables based on your conditions and use a switch case, or use else if

  2. Do not compare a string "nitrogen" to a number 0

See : http://codepad.org/8a4qFgmf

.

   <?php
    $myVar = ('Nitrogen' == 0);
    var_dump($myVar);  // THIS IS TRUE
    ?>
DhruvPathak
  • 42,059
  • 16
  • 116
  • 175
  • I tried both elseif and switch, the result is same, no change. I am not comparing string to a number, my 2 variables could have numeric or text comparing the same numeric or text. I am not sure how to use the example given above by you, as my both the variables keep varying the values, based on user input from 2 dropdown lists. – user1114409 Jan 11 '13 at 06:42
1

How about doing something more like this (though this is still not safe for user defined data (needs to be properly escaped)

<?php

function array_map_assoc( $callback , $array ){
    $r = array();
      foreach ($array as $key=>$value)
            $r[$key] = $callback($key,$value);
        return $r;
}

function funtimes($option, $suboption) {
  $query = "SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER%sORDER BY Ac_code, Prod_desc";
  $clause = array("Ac_code" => $option, "Prod_desc" => $suboption);
  $clause = array_filter($clause);
  $where = ' ';
  if (count($clause)) {
    $where = " WHERE " . implode(', ',array_map_assoc(function($k,$v){return "$k='$v'";},$clause)) . " ";
  }
  echo "For option=$option, suboption=$suboption\n";
  echo sprintf($query, $where);
  echo "\n\n";
}

funtimes(0, 0);
funtimes(1, 0);
funtimes(0, 1);
funtimes(1, 1);

OUTPUT

For option=0, suboption=0
SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER ORDER BY Ac_code, Prod_desc

For option=1, suboption=0
SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER WHERE Ac_code='1' ORDER BY Ac_code, Prod_desc

For option=0, suboption=1
SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER WHERE Prod_desc='1' ORDER BY Ac_code, Prod_desc

For option=1, suboption=1
SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER WHERE Ac_code='1', Prod_desc='1' ORDER BY Ac_code, Prod_desc

Now you have the right query, so just execute it.

sberry
  • 128,281
  • 18
  • 138
  • 165
  • +1,,,, this is best solution i saw ever for this switchwation.. have to read twice to understand what is happening .. – NullPoiиteя Jan 11 '13 at 06:39
  • @sberry Need not complicate code when it can be done in simple way, hope you understand. BUT still your code is not solving my problem. Prod_desc is Product Description, it would have only text, so I need to compare only text for $suboption. – user1114409 Jan 11 '13 at 06:56
1

Yes the code can be cleaned up, but to your code, if your inputs are numbers as well as text AND if 0 denotes no option, and empty string denotes no option (which seems like what you are doing), then replace conditions as

if ($option==0 && $suboption==0)
to
if ($empty(option) && empty($suboption))

and likewise.

I would have written code like this

$qry = 'SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER';
$where = ' WHERE ';
$post = ' ORDER BY Ac_code, Prod_desc';
$clause = '';
if(!empty($option))
{
    $clase = $where . " Ac_code='$option'";
    $where = " AND ";
}
if(!empty($suboption))
{
    $clase = $where . " Prod_desc='$suboption'";
}
Amit
  • 1,836
  • 15
  • 24
0

While I agree with DhruvPathak, here is your code cleaned up a bit. Switch may not be the best choice because you are comparing 2 things, not just one.

//initialize variables
$option = $suboption = 0;

// get option values
//...your code here...

$sql = "SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER";
if ($option!=0 && $suboption==0) {
   $sql .= " WHERE Ac_code='$option'";
}
elseif($option==0 && $suboption!=0) {
   $sql .= " WHERE Prod_desc='$suboption'";
}
elseif($option!=0 && $suboption!=0) {
   $sql .= " WHERE Ac_code='$option' AND Prod_desc='$suboption'";
}
$sql .= " ORDER BY Ac_code, Prod_desc";
$dc = mysql_query($sql);

Note that these queries are open to SQL injection attack if you are getting the option and suboption values from via a GET or POST. There are also better ways of doing the data fetch.

Based on your feedback, does this work?

$sql = "SELECT Ac_code, Prod_desc, Capacity FROM RATEMASTER";
if ($option!=0) {
   $sql .= " WHERE Ac_code='$option'";
   if ( (int)$suboption >= 0 || strlen($suboption) > 1)
      $sql .= " AND Prod_desc='$suboption'";
}
elseif($option==0 && ((int)$suboption >= 0 || strlen($suboption) > 1) ) {
   $sql .= " WHERE Prod_desc='$suboption'";
}
$sql .= " ORDER BY Ac_code, Prod_desc";
Revent
  • 2,091
  • 2
  • 18
  • 33
  • Your code is very simple and works fine ONLY when I have both the varibles comparing numeric values, what I need is when $option is comparing numeric 0 or 1,2... and $suboption could have either numeric 0 or text like Nitrogen or Oxygen. This is when it does not work. – user1114409 Jan 11 '13 at 06:52
  • 1
    Then instead of comparing $suboption just to zero, you could add a check for either a positive integer or the length of the string. If the text can only be certain values, then you could use an array to check for those. – Revent Jan 11 '13 at 06:55
  • You are right, if $suboption is not checked for only zero, and if I check the length of string. it is working fine. SO THIS WAS THE ISSUE and not the NESTED IF ELSE. – user1114409 Jan 11 '13 at 07:04