Solved

Help refactoring if/elseif

Posted on 2010-11-23
9
355 Views
Last Modified: 2012-08-13
I have the code below written by someone else. It is a little confusing
and I would like to refactor it to look better and be more logical.

The SQL has been changed slightly to mask what it is really doing, it is
just the if/elesif's that I am concerned about because the whole construct
looks confusing.

From what I'm seeing, the code inside the "if($cat != "")" inside the elseif's
at lines 8, 15 and 22 will never run because of the first "if($cat != "")" at
line 1. i.e. if $flag == 1 and $cat == 'book' then it would always fall into
the first if at line 1 and never reach the sql at line 9.

Is this a correct assumption?

How would you refactor this code?

 1  if($cat != "") {
 2    $sel_gig="SELECT * FROM table 
 3              WHERE category_id=".$cat;
 4  } elseif($mny != "") {
 5    $sel_gig="SELECT * FROM table 
 6              WHERE price=".$mny
 7  } elseif($flag == "l") {
 8    if($cat != "") {
 9      $sel_gig="SELECT * FROM table 
10                WHERE category_id=".$cat;
11    } else {
12      $sel_gig="SELECT * FROM table";
13    }
14  } elseif($flag == "p") {
15    if($cat != "") {
16      $sel_gig="SELECT * FROM `clientorder`
17                WHERE category_id=".$cat";
18    } else {
19      $sel_gig="SELECT * FROM `clientorder`:;
20    }
21  } elseif($flag == "r11") {
22    if($cat != "") {
23      $sel_gig="SELECT * FROM `feedback`
24                WHERE category_id=".$cat;
25    } else {
26      $sel_gig="SELECT * FROM `feedback`";
27    }
28  }

Open in new window

0
Comment
Question by:EddieShipman
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
9 Comments
 
LVL 3

Expert Comment

by:chrios
ID: 34197231
You can remove all elseif ($cat != "") since that condition is handled on the first line.

That leaves the code below. Maybe you want to add a final else to the list, but this is the cleaned up version.
if($cat != "") {
    $sel_gig="SELECT * FROM table WHERE category_id=".$cat;
 } elseif($mny != "") {
   $sel_gig="SELECT * FROM table WHERE price=".$mny
 } elseif($flag == "l") {
     $sel_gig="SELECT * FROM table";
 } elseif($flag == "p") {
     $sel_gig="SELECT * FROM `clientorder`:;
 } elseif($flag == "r11") {
     $sel_gig="SELECT * FROM `feedback`";
 }

Open in new window

0
 
LVL 3

Expert Comment

by:chrios
ID: 34197269
Perhaps I should clarify my last post.

Since line 1 is "if($cat != "") {..." you know that in the rest of the elseif-code that condition must be False, therefor you can remove all the "if($cat != "") { xxx } " that follows in the code since it's redundant.
0
 
LVL 26

Author Comment

by:EddieShipman
ID: 34197270
Well, I don't think I can do that. here's the whole code, with different column, table names.
if($cat != "") {
    $sel_table="SELECT * FROM table 
              WHERE category_id=".$cat." 
              LIMIT $start,$max";
  } elseif($mny != "") {
    $sel_table="SELECT * FROM  table 
              WHERE table_price=".$mny." 
              LIMIT $start,$max";
  } elseif($flag == "l") {
    if($cat != "") {
      $sel_table="SELECT * FROM  table 
                WHERE category_id=".$cat." 
                ORDER BY id_col DESC LIMIT $start,$max";
    } else {
      $sel_table="SELECT * FROM  table 
                ORDER BY id_col DESC LIMIT $start,$max";
    }
  } elseif($flag == "p") {
    if($cat != "") {
      $sel_table="SELECT `order_id_col`, COUNT(`order_id_col`) AS pop, table.* FROM `order`, table 
                WHERE id_col=order_id_col AND category_id=".$cat." 
                GROUP BY `order_id_col` ORDER BY pop DESC LIMIT $start,$max";
    } else {
      $sel_table="SELECT `order_id_col`, COUNT(`order_id_col`) AS pop, table.* FROM `order`, table 
                WHERE id_col=order_id_col GROUP BY `order_id_col` 
                ORDER BY pop DESC LIMIT $start,$max";
    }
  } elseif($flag == "r11") {
    if($cat != "") {
      $sel_table="SELECT feedback.id_col,count(feedback.id_col) AS pop, table.* FROM `feedback`, table 
                WHERE feedback.id_col = table.id_col AND category_id=".$cat." 
                GROUP BY feedback.id_col 
                ORDER BY pop DESC LIMIT $start,$max";
    } else {
      $sel_table="SELECT feedback.id_col,count(feedback.id_col) AS pop, table.* FROM `feedback`, table 
                WHERE feedback.id_col = table.id_col 
                GROUP BY feedback.id_col 
                ORDER BY pop DESC LIMIT $start,$max";
    }
  }

Open in new window

0
MS Dynamics Made Instantly Simpler

Make Your Microsoft Dynamics Investment Count  & Drastically Decrease Training Time by Providing Intuitive Step-By-Step WalkThru Tutorials.

 
LVL 11

Assisted Solution

by:level9wizard
level9wizard earned 150 total points
ID: 34197342
Are you trying to have 8 potential separate SELECT statements? Or are you trying to build a single select statement (for example that includes a WHERE statement on category_id and price)?
0
 
LVL 26

Author Comment

by:EddieShipman
ID: 34197614
I just want it refactored so I can understand it, it is a real jumble now.
0
 
LVL 34

Assisted Solution

by:Beverley Portlock
Beverley Portlock earned 150 total points
ID: 34197642
OK - What I have done is to load the code into Eclipse, break the elseif statements into else if statements instead and then hit "reformat source" which promptly turned up several missing quotes and then reformatted the code as shown below.

I think this layout makes it much easier to see what is gong on.

 
<?php


if ($cat != "") {
	$sel_gig = "SELECT * FROM table 
               WHERE category_id=".$cat;
} else
	if ($mny != "") {
		$sel_gig = "SELECT * FROM table 
		               WHERE price=".$mny} else
		if ($flag == "l") {
			if ($cat != "") {
				$sel_gig = "SELECT * FROM table  WHERE category_id=".$cat;
			} else {
				$sel_gig = "SELECT * FROM table";
			}
		} else
			if ($flag == "p") {
				if ($cat != "") {
					$sel_gig = "SELECT * FROM `clientorder`  WHERE category_id=".$cat;
				} else {
					$sel_gig = " SELECT * FROM `clientorder` :";
				}
			} else
				if ($flag == " r11 ") {
					if ($cat != " ") {
						$sel_gig = " SELECT * FROM `feedback` WHERE category_id = ".$cat;
					} else {
						$sel_gig = " SELECT * FROM `feedback`";
					}
				}

Open in new window




I would then remove brackets around single statements to reduce the clutter further

 
<?php
if ($cat != "")
	$sel_gig = "SELECT * FROM table  WHERE category_id=".$cat;
else
	if ($mny != "")
		$sel_gig = "SELECT * FROM table WHERE price=".$mny;
	else
		if ($flag == "l") {
			if ($cat != "")
				$sel_gig = "SELECT * FROM table  WHERE category_id=".$cat;
			else
				$sel_gig = "SELECT * FROM table";

		} 
          else {
			if ($flag == "p") {
				if ($cat != "")
					$sel_gig = "SELECT * FROM `clientorder`  WHERE category_id=".$cat;
				else
					$sel_gig = " SELECT * FROM `clientorder` :";

			} 
               else
				if ($flag == " r11 ") 
					if ($cat != " ")
						$sel_gig = " SELECT * FROM `feedback` WHERE category_id = ".$cat;
					else
						$sel_gig = " SELECT * FROM `feedback`";

          }

Open in new window



Now I cab see what's going on. The first statement is

if ( $cat != "")

which means that if you ever get in to the ELSE then $cat == "" so there is no point in testing $cat in the ELSE because you know it is blank.



<?php

if ($cat != "")
	$sel_gig = "SELECT * FROM table  WHERE category_id=".$cat;
else
	if ($mny != "")
		$sel_gig = "SELECT * FROM table WHERE price=".$mny;
	else
		if ($flag == "l") 
			$sel_gig = "SELECT * FROM table";
		else
			if ($flag == "p")
				$sel_gig = " SELECT * FROM `clientorder` :";
			else
				if ($flag == " r11 ")
					$sel_gig = " SELECT * FROM `feedback`";

Open in new window

0
 
LVL 3

Expert Comment

by:chrios
ID: 34197662
This is equivalent to the code you posted the second time. As you said yourself in your question, the if($cat != "") on lines 10, 19, 29 can never be True because of line 1.


if($cat != "") {
    $sel_table="SELECT * FROM table 
              WHERE category_id=".$cat." 
              LIMIT $start,$max";
} elseif($mny != "") {
    $sel_table="SELECT * FROM  table 
              WHERE table_price=".$mny." 
              LIMIT $start,$max";
} elseif($flag == "l") {
      $sel_table="SELECT * FROM  table 
                ORDER BY id_col DESC LIMIT $start,$max";
} elseif($flag == "p") {
       $sel_table="SELECT `order_id_col`, COUNT(`order_id_col`) AS pop, table.* FROM `order`, table 
                WHERE id_col=order_id_col GROUP BY `order_id_col` 
                ORDER BY pop DESC LIMIT $start,$max";
} elseif($flag == "r11") {
       $sel_table="SELECT feedback.id_col,count(feedback.id_col) AS pop, table.* FROM `feedback`, table 
                WHERE feedback.id_col = table.id_col 
                GROUP BY feedback.id_col 
                ORDER BY pop DESC LIMIT $start,$max";
}

Open in new window

0
 
LVL 26

Author Comment

by:EddieShipman
ID: 34198240
If you move the first if to an elseif at the end, I think it will work like they intended.
if($mny != "") {
    $sel_table="SELECT * FROM  table 
              WHERE table_price=".$mny." 
              LIMIT $start,$max";
  } elseif($flag == "l") {
    if($cat != "") {
      $sel_table="SELECT * FROM  table 
                WHERE category_id=".$cat." 
                ORDER BY id_col DESC LIMIT $start,$max";
    } else {
      $sel_table="SELECT * FROM  table 
                ORDER BY id_col DESC LIMIT $start,$max";
    }
  } elseif($flag == "p") {
    if($cat != "") {
      $sel_table="SELECT `order_id_col`, COUNT(`order_id_col`) AS pop, table.* FROM `order`, table 
                WHERE id_col=order_id_col AND category_id=".$cat." 
                GROUP BY `order_id_col` ORDER BY pop DESC LIMIT $start,$max";
    } else {
      $sel_table="SELECT `order_id_col`, COUNT(`order_id_col`) AS pop, table.* FROM `order`, table 
                WHERE id_col=order_id_col GROUP BY `order_id_col` 
                ORDER BY pop DESC LIMIT $start,$max";
    }
  } elseif($flag == "r11") {
    if($cat != "") {
      $sel_table="SELECT feedback.id_col,count(feedback.id_col) AS pop, table.* FROM `feedback`, table 
                WHERE feedback.id_col = table.id_col AND category_id=".$cat." 
                GROUP BY feedback.id_col 
                ORDER BY pop DESC LIMIT $start,$max";
    } else {
      $sel_table="SELECT feedback.id_col,count(feedback.id_col) AS pop, table.* FROM `feedback`, table 
                WHERE feedback.id_col = table.id_col 
                GROUP BY feedback.id_col 
                ORDER BY pop DESC LIMIT $start,$max";
    }
  } elseif($cat != "") {
    $sel_table="SELECT * FROM table 
              WHERE category_id=".$cat." 
              LIMIT $start,$max";
  }

Open in new window

0
 
LVL 3

Accepted Solution

by:
chrios earned 200 total points
ID: 34199079
If you move the first if to an elseif at the end, I think it will work like they intended.
I agree, that makes more sense.
0

Featured Post

Free Tool: ZipGrep

ZipGrep is a utility that can list and search zip (.war, .ear, .jar, etc) archives for text patterns, without the need to extract the archive's contents.

One of a set of tools we're offering as a way to say thank you for being a part of the community.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

Title # Comments Views Activity
Ajax and PHP 9 53
Could you point what's preventing a remote MySQL server to be accessed? 8 40
Survey branching tutorial 11 44
Special characters in a TCPDF 4 27
Generating table dynamically is the most common issue faced by php developers.... So it seems there is a need of an article that explains the basic concept of generating tables dynamically. It just requires a basic knowledge of html and little maths…
This article discusses how to implement server side field validation and display customized error messages to the client.
The viewer will learn how to look for a specific file type in a local or remote server directory using PHP.
The viewer will learn how to create and use a small PHP class to apply a watermark to an image. This video shows the viewer the setup for the PHP watermark as well as important coding language. Continue to Part 2 to learn the core code used in creat…

733 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question