0
\$\begingroup\$

Okay so I built this search page that searches for posts but my current code seems so bloated and inefficient.

My script(search page)

if(isset($_GET['title']))$title = $_GET['title'];else{ $title = ''; }
if(isset($_GET['ad_brand']))$brand = $_GET['ad_brand'];else{ $brand = ''; }
if(isset($_GET['min_range']))$min_range = $_GET['min_range'];else{ $min_range = ''; }
if(isset($_GET['max_range']))$max_range = $_GET['max_range'];else{ $max_range = ''; }
if(isset($_GET['sub_cat']))$sub_cat = $_GET['sub_cat'];else{ $sub_cat = ''; }
if(isset($_GET['for_r_s']))$for_r_s = $_GET['for_r_s']; else{ $for_r_s = ''; }
if(isset($_GET['main_cat']))$main_cat = $_GET['main_cat']; else{ $main_cat = ''; }

if(isset($_POST['filter_button'])){
    if(isset($_POST['ad_brand']))
        $brand = $_POST['ad_brand'];
    else{$brand = '';}

    if(isset($_POST['main_cat']))
    $main_cat = $_POST['main_cat'];
    else{ $main_cat = ''; }

    if(isset($_POST['sub_cat']))
        $sub_cat = $_POST['sub_cat'];
    else{ $sub_cat = ''; }

    if($_POST['min_range'] != '')
        $min_range = $_POST['min_range'];
    else{ $min_range = ''; }

    if(isset($_POST['max_range']))
        $max_range = $_POST['max_range'];
    else{ $max_range = ''; }


    if(isset($_POST['for_r_s']))
        $for_r_s = $_POST['for_r_s'];
    else{ $for_r_s = ''; }
    header('location:search?title='. $title .'&main_cat='.$main_cat.'&sub_cat='. $sub_cat .'&ad_brand='. $brand .'&min_range='.$min_range.'&max_range='.$max_range.'&for_r_s='.$for_r_s.'');
}

if(isset($_GET['sub_cat'])){
    if($_GET['sub_cat'] != ''){
            if(isset($_GET['min_range'])){
                if(isset($_GET['max_range']) && $_GET['max_range'] != ''){
                    if(isset($_GET['for_r_s'])){
                        if($_GET['for_r_s'] == 1 || $_GET['for_r_s'] == 2){
                            $query = "SELECT * FROM `posts` 
                            WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ?
                            AND `ad_brand` LIKE CONCAT('%',?,'%') AND `ad_price` >= ? 
                            AND `ad_price` <= ? AND `for_r_s` = ?"; 
                            $get_posts = $conn_posts->prepare($query);
                            $get_posts->bind_param("sssiii", $title, $sub_cat ,$brand, $min_range, $max_range, $for_r_s);  
                        }else{
                            $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` 
                            LIKE CONCAT('%',?,'%') AND `ad_price` >= ? AND `ad_price` <= ?"; 
                            $get_posts = $conn_posts->prepare($query);
                            $get_posts->bind_param("sssii", $title, $sub_cat ,$brand, $min_range, $max_range);  
                        }
                    }else{
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` 
                        LIKE CONCAT('%',?,'%') AND `ad_price` >= ? AND `ad_price` <= ?"; 
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("sssii", $title, $sub_cat ,$brand, $min_range, $max_range);  
                    }
                }else{
                    if(isset($_GET['for_r_s'])){
                        if($_GET['for_r_s'] == 1 || $_GET['for_r_s'] == 2){
                            $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` 
                            LIKE CONCAT('%',?,'%') AND `ad_price` >= ? AND `for_r_s` = ?"; 
                            $get_posts = $conn_posts->prepare($query);
                            $get_posts->bind_param("sssii", $title, $sub_cat ,$brand, $min_range, $for_r_s);
                        }else{
                            $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` 
                            LIKE CONCAT('%',?,'%') AND `ad_price` >= ?"; 
                            $get_posts = $conn_posts->prepare($query);
                            $get_posts->bind_param("sssi", $title, $sub_cat ,$brand, $min_range);
                        }
                    }else{
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` 
                        LIKE CONCAT('%',?,'%') AND `ad_price` >= ?"; 
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("sssi", $title, $sub_cat ,$brand, $min_range);
                    }
                }
            }else{
                if(isset($_GET['max_range']) && $_GET['max_range'] != ''){
                    if(isset($_GET['for_r_s'])){
                        if($_GET['for_r_s'] == 1 || $_GET['for_r_s'] == 2){
                            $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` 
                            LIKE CONCAT('%',?,'%') AND `ad_price` <= ? AND `for_r_s` = ?"; 
                            $get_posts = $conn_posts->prepare($query);
                            $get_posts->bind_param("sssii", $title, $sub_cat ,$brand, $max_range,$for_r_s);
                        }else{
                            $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` 
                            LIKE CONCAT('%',?,'%') AND `ad_price` <= ?"; 
                            $get_posts = $conn_posts->prepare($query);
                            $get_posts->bind_param("sssi", $title, $sub_cat ,$brand, $max_range);
                        }
                    }else{
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` 
                        LIKE CONCAT('%',?,'%') AND `ad_price` <= ?"; 
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("sssi", $title, $sub_cat ,$brand, $max_range);
                    }
                }else{
                    if(isset($_GET['for_r_s'])){
                        if($_GET['for_r_s'] == 1 || $_GET['for_r_s'] == 2){
                            $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` 
                            LIKE CONCAT('%',?,'%') AND `for_r_s` = ?";
                            $get_posts = $conn_posts->prepare($query);
                            $get_posts->bind_param("sssi", $title, $sub_cat ,$brand, $for_r_s);
                        }else{
                            $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` LIKE CONCAT('%',?,'%')";
                            $get_posts = $conn_posts->prepare($query);
                            $get_posts->bind_param("sss", $title, $sub_cat ,$brand);
                        }
                    }else{
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_sub_cat` = ? AND `ad_brand` LIKE CONCAT('%',?,'%')";
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("sss", $title, $sub_cat ,$brand); 
                    }
                }
            }
    }else{
        if(isset($_GET['min_range'])){
            if(isset($_GET['max_range']) && $_GET['max_range'] != ''){
                if(isset($_GET['for_r_s'])){
                    if($_GET['for_r_s'] == 1 || $_GET['for_r_s'] == 2){
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` 
                        LIKE CONCAT('%',?,'%') AND `ad_price` >= ? AND `ad_price` <= ? AND `for_r_s` = ?"; 
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("ssiii", $title, $brand, $min_range, $max_range, $for_r_s);
                    }else{
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` 
                        LIKE CONCAT('%',?,'%') AND `ad_price` >= ? AND `ad_price` <= ?"; 
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("ssii", $title, $brand, $min_range, $max_range);
                    }
                }else{
                    $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` 
                    LIKE CONCAT('%',?,'%') AND `ad_price` >= ? AND `ad_price` <= ?"; 
                    $get_posts = $conn_posts->prepare($query);
                    $get_posts->bind_param("ssii", $title, $brand, $min_range, $max_range);
                }
            }else{
                if(isset($_GET['for_r_s'])){
                    if( $_GET['for_r_s'] == 1 || $_GET['for_r_s'] == 2){
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` 
                        LIKE CONCAT('%',?,'%') AND `ad_price` >= ? AND `for_r_s` = ?"; 
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("sssi", $title, $brand, $min_range, $for_r_s);
                    }else{
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` 
                        LIKE CONCAT('%',?,'%') AND `ad_price` >= ?"; 
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("ssi", $title, $brand, $min_range);
                    }
                }else{
                    $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` 
                    LIKE CONCAT('%',?,'%') AND `ad_price` >= ?"; 
                    $get_posts = $conn_posts->prepare($query);
                    $get_posts->bind_param("ssi", $title, $brand, $min_range); 
                }
            }
        }else{
            if(isset($_GET['max_range']) && $_GET['max_range'] != ''){
                if(isset($_GET['for_r_s'])){
                    if($_GET['for_r_s'] == 1 || $_GET['for_r_s'] == 2){
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` 
                        LIKE CONCAT('%',?,'%') AND `ad_price` <= ? AND `for_r_S` = ?"; 
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("ssii", $title, $brand, $max_range, $for_r_s);
                    }else{
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` 
                        LIKE CONCAT('%',?,'%') AND `ad_price` <= ?"; 
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("ssi", $title, $brand, $max_range); 
                    }
                }else{
                    $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` 
                    LIKE CONCAT('%',?,'%') AND `ad_price` <= ?"; 
                    $get_posts = $conn_posts->prepare($query);
                    $get_posts->bind_param("ssi", $title, $brand, $max_range); 
                }
            }else{
                if(isset($_GET['for_r_s'])){
                    if($_GET['for_r_s'] == 1 || $_GET['for_r_s'] == 2){
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` LIKE CONCAT('%',?,'%') AND `for_r_s` = ?";
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("ssi", $title, $brand, $for_r_s);
                    }else{
                        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` LIKE CONCAT('%',?,'%')";
                        $get_posts = $conn_posts->prepare($query);
                        $get_posts->bind_param("ss", $title, $brand); 
                    }
                }else{
                    $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` LIKE CONCAT('%',?,'%')";
                    $get_posts = $conn_posts->prepare($query);
                    $get_posts->bind_param("ss", $title, $brand); 
                }
            }
        }
    }
}else{
    $sub_cat = ''; 
    if(isset($_GET['for_r_s'])){
        if($_GET['for_r_s'] == 1 || $_GET['for_r_s'] == 2){
            $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` LIKE CONCAT('%',?,'%') AND `for_r_s` = ?";
            $get_posts = $conn_posts->prepare($query);
            $get_posts->bind_param("ssi", $title, $brand, $for_r_s);
        }else{
            $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` LIKE CONCAT('%',?,'%')";
            $get_posts = $conn_posts->prepare($query);
            $get_posts->bind_param("ss", $title, $brand);
        }
    }else{
        $query = "SELECT * FROM `posts` WHERE `ad_title` LIKE CONCAT('%',?,'%') AND `ad_brand` LIKE CONCAT('%',?,'%')";
        $get_posts = $conn_posts->prepare($query);
        $get_posts->bind_param("ss", $title, $brand); 
    }
}

My Html code

 <form action="" method="post">
                    <select name="main_cat" id="main_cat">
                        <option value="null" disabled selected>Select Category</option>
                        <?php 
                            $get_category = $conn_posts->prepare("SELECT * FROM `cats` WHERE `main_cat` = 0;");
                            $get_category->execute();
                            $get_category_results = $get_category->get_result(); // get result
                            while($row = $get_category_results->fetch_assoc()){ 
                                echo '<option value="'.$row['ID'].'">'.$row['cat_name'].'</option>';
                            }
                        ?>
                    </select>
                    </br>
                    <select name="sub_cat" id="sub-category-dropdown">
                        <option value="">Select SubCategory</option>
                    </select>
                    </br>
                    <input type="text" name="ad_brand" Placeholder="Brand(Cat,Jcb,Doosan)">
                    </br>
                    <label for="for_r_S">Price</label>
                    <div class="range_sliders">
                        <input type="text" name="min_range" Placeholder="Min">
                        <span> - </span>
                        <input type="text" name="max_range" Placeholder="Max">
                    </div>
                    </br>
                    <label for="for_r_S">For Rent</label>
                    <input type="radio" name="for_r_s" id="for_r_s" value="1">
                    <label for="for_r_S">For Sale</label>
                    <input type="radio" name="for_r_s" id="for_r_s" value="2">
                    </br>
                    <div id="content"></div>
                    <button type="submit" name="filter_button" class="filter_button">Search</button> 
                </form>

My Database table layout

ID ad_title ad_sub_cat ad_price used_new for_r_s ad_brand ad_des ad_location
1 title 12 100 1 1 loerm ipsum new york

for_r_s:: 1 = for rent, 2 = for sale

used_new:: 1 = used, 2 = new

Also the sub cat is gotten from ajax but i don't want to include that code here so just fill it in with any category like this <option value="12">Select car</option>

The script is working how I want it to but I just want to clean this code(i don't know of any bugs and it should work just fine) and I need some help doing that my friend told me to ask here so I am still ned please tell me if this post is not good and i will put more info.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Could you elaborate a bit more on what this code does? The only thing we know, by combining two things you wrote, is: "It searches for posts using multiple filter inputs.". We have no idea what the data in your database represents. You also have a whole maze of if () {...} else {...} elseif () {...} in your code that makes little sense. What does that do, and why? Understanding code can be so much easier if the idea behind it is known. Remember, we have no data, we cannot run your code, all we have is the information you give. \$\endgroup\$ Nov 5, 2021 at 12:04

2 Answers 2

2
\$\begingroup\$

There is one thing that is not right in your application: tabs and indentation.

Example:

if(isset($_POST['filter_button'])){
    if(isset($_POST['ad_brand']))
        $brand = $_POST['ad_brand'];
    else{$brand = '';}

So the code is hard to read and decipher because it is too compact, indentation and spacing are not consistent and the control flows are not outlined clearly enough.

Something like this would be slightly more readable:

if(isset($_POST['filter_button'])) {

    if(isset($_POST['ad_brand'])) {
        $brand = $_POST['ad_brand'];
    else {
        $brand = '';
    }

Right ?

But since PHP has the ternary operator the code could be written more concisely in a one-liner fashion like:

$brand = (isset($_POST['ad_brand'])) ? $_POST['ad_brand'] : '';

The other problem is that you have too many nested ifs. From line 38, you have 6 levels of nesting. This is too much for a program that is not even of high complexity. It is very hard to figure out if the logic is even correct, because it is difficult to tell where the ifs exactly begin end end. The likelihood of producing bugs becomes high as a result.

To simplify this my advice is to check the input values early, provide default values where desired, (or abort with an error if the form is being tampered by the user). In fact you are already doing it. If you consider for example lines 38-39:

if(isset($_GET['sub_cat'])){
    if($_GET['sub_cat'] != ''){

the form field sub_cat has already been assigned to variable $sub_cat previously. So from now on, you should be using that variable. It has been validated and has a default value too, so there is no need to make further verification. So you can already remove two levels of nesting.


The third problem is repetition. It is understandable that you want to have different SQL statements depending on some condition, but there is no need to repeat prepare, bind_param etc. Once would be enough, at the end of the if block.

But you should go further and move all the SQL statements to dedicated functions. What you need is a function with a few parameters, some of them optional, with default values for some parameters, and inside that function you can build your SQL statement.

Eg:

function get_posts($title, $subcat, $min_price, $max_price) { ... }

The function shall return a resultset depending on the supplied parameters. All you have to do is call the function with appropriate values. Inside that function you can have ifs and build a dynamic statement according to the function arguments received.

Basically all you have is a series of criteria. If for a example a minimum price is supplied to your function you can concatenate this to your SQL statement:

$query = "SELECT * FROM `posts`";

if ($min_price) {
    $query .= " AND price >= $min_price";
}

and so on, as long as the resulting SQL remains correct. So what you will be doing is selective concatenation. As far as I can tell the criteria can be evaluated one by one in a sequential manner. All you will need is some ifs, AND, OR. The idea is to concentrate all the logic into that function. Thus you could ditch all those if blocks.

Just doing that will reduce the code size significantly and improve readability. The rule of thumb that everything that is repetitive is a candidate for a function.

To sum up, all you need is:

  • verify that all expected parameters are received from a POST request
  • validate them
  • where appropriate, you can supply default values (you are already doing that)
  • populate your variables
  • then you can pass those variables to a function that will build your SQL dynamically

Result: all the nested ifs are gone.


Something that could make the code more explanatory is to use constants. eg:

define('FOR_SALE', '1');
define('FOR_RENT', '2');

Then you can use variables like FOR_SALE or FOR_RENT in your code to make the SQL statements more descriptive.


What is disturbing is the total lack of comments. It would be good to add some notes in plain English here and there, especially when you are checking for some condition. The idea is to better separate blocks, and that if you are looking for something, you don't have to decipher the whole code.


The database structure is not known but judging by the present of LIKE and CONCAT I suspect that you are not taking advantage of indexes, provided they are present. You may run into performance issues as your database grows bigger.


The HTML page on the other hand is easy. You may want to use a templating engine like twig perhaps.


PS: maybe you need a better editor to enforce good formatting.

\$\endgroup\$
6
  • \$\begingroup\$ thanks bud i actually found a nice way of doing it and i started over as my previous code had way to many if stmts lol i don't know what I was even thinking but thanks for all the info and I will keep all of it in my mind while i code my new page :) you gave me wonderful info and this has to be the best answer by far - hope you have a great day \$\endgroup\$ Nov 5, 2021 at 18:10
  • \$\begingroup\$ if you are wondering I am thinking of using this type of code stackoverflow.com/a/13207064/17239314 i know its from 9yrs ago but I am updating the code so that it won't be vulnerable to sql injection \$\endgroup\$ Nov 5, 2021 at 18:12
  • \$\begingroup\$ do you think that is a valid solution? \$\endgroup\$ Nov 6, 2021 at 13:56
  • \$\begingroup\$ Something along these lines, sequential concatenation, as long as you take care not introduce SQL injection vulnerabilities. Use bind_param one parameter at a time. \$\endgroup\$
    – Kate
    Nov 6, 2021 at 20:46
  • \$\begingroup\$ yup will do thx bud \$\endgroup\$ Nov 8, 2021 at 14:37
1
\$\begingroup\$

If you are using a new enough PHP version:

if(isset($_GET['max_range']))
    $max_range = $_GET['max_range'];
    else{ $max_range = ''; }

becomes simply

$max_range = $_GET['max_range'] ?? '';

See Null Coalescing Operator in the manual.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.