need help improving mysql and php filter code

Question

I only recently started coding so I know my code is not very good, but i would really appreciate any help on improving my code.

rating |image_location |description

the first page this page is pretty straightforward your typical search that links to the search page and passes the keywords entered through GET

<div id=\"top_search\">
            <form name=\"input\" action=\"search.php\" method=\"get\" id=\"search_form\">
            <input type=\"text\" id=\"keywords\" name=\"keywords\" size=\"128\" class=\"searchbox\" value=\"$defaultText\"> &nbsp;
            <select id=\category\" name=\"category\" class=\"searchbox\"> 
";
createCategoryList();
echo '
            </select> &nbsp;
            <input type="submit" value="Search" class="button" /> &nbsp;
            </form>
        </div>

the second page is the search page this is where things got complicated for me.Here is what i did i created a buch of links on the left side, each link submits back to the same page but passes 4 variables with them which i use on the third page to create the query.As you can see its not very pretty.

<?php
session_start();
include("includes/html_codes.php");
include ("includes/search_func.php");

if (isset($_GET['keywords'])){
$keywords = mysql_real_escape_string(htmlentities(trim($_GET['keywords']))); 
}
if (isset($_GET['order'])){
$order = mysql_real_escape_string(htmlentities(trim($_GET['order']))); 
}else{
    $order = '';
}

if (isset($_GET['platform'])){
$platform = mysql_real_escape_string(htmlentities(trim($_GET['platform']))); 
}else{
    $platform = '';
}

if (isset($_GET['genre'])){
$genre = mysql_real_escape_string(htmlentities(trim($_GET['genre']))); 
}else{
    $genre = '';
}
$errors = array();

if(empty($keywords)){
    $errors[] = 'Please enter a search term';
}else if(strlen($keywords)<3){
    $errors[] = 'Your search term must be at least 3 characters long';
}else if(search_results($keywords) == false){
            $errors[] = 'Your search for'.$keywords.' returned no results';
}
if(empty($errors)){

    $results = search_results($keywords);
    $results_num = count($results);
}else{
    foreach($errors as $error){
        echo $error, '<br />';
    }
}
?>

 <!DOCTYPE html >

<html lang="en">
<head>
<title>Search</title>
<link rel="stylesheet" href="css/main.css">
<link rel="stylesheet" href="css/search.css">
</head>
<body>
<?php topBanner(); ?>   
<div id="wrapper">
<?php headerAndSearchCode(); 
echo $_SERVER['QUERY_STRING'];
echo $keywords;
?>
    <div id="main_section" class="header">
        <?php       

        echo '
        <div id="filter_nav">
        <ul  id="nav_form">
            <li><h3 id="h3">Genre: &nbsp;</h3>
            <li><a href="search.php?keywords='.$keywords.'&platform='.$platform.'&genre=Fighting&order='.$order.'">Fighting</a></li>
            <li><a href="search.php?keywords='.$keywords.'&platform='.$platform.'&genre=Role-Playing&order='.$order.'">Role-Playing</a></li>
            <li><a href="search.php?keywords='.$keywords.'&platform='.$platform.'&genre=Action&order='.$order.'">Action</a></li>
        </ul>
        <ul  id="nav_form">
            <li><h3 id="h3">Platform: &nbsp;</h3>
            <li><a href="search.php?keywords='.$keywords.'&platform=Playstation 3&genre='.$genre.'&order='.$order.'">PS3</a></li>
            <li><a href="search.php?keywords='.$keywords.'&platform=xbox 360&genre='.$genre.'&order='.$order.'">Xbox 360</a></li>
            <li><a href="search.php?keywords='.$keywords.'&platform=Gamecube&genre='.$genre.'&order='.$order.'">Gamecube</a></li>
        </ul>
        </div>
    ';
        echo '
        <ul  id="sorting_form">
            <li><h3 id="h3">SORT BY: &nbsp;</h3>
            <li><a href="search.php?keywords='.$keywords.'&platform='.$platform.'&genre='.$genre.'&order=title">Title</a></li>
            <li><a href="search.php?keywords='.$keywords.'&platform='.$platform.'&genre='.$genre.'&order=release_date">Date</a></li>
            <li><a href="search.php?keywords='.$keywords.'&platform='.$platform.'&genre='.$genre.'&order=rating">Rating</a></li>

        </ul>
        ';
        echo '<div id="results">';
    foreach($results as $result ){
        echo '
            <div id="game_result">
                <a href= game_page.php?game_id='.$result['game_id'].'><img src= '.$result['image_location'].' id="image" /></a>
                <div id="main_title">
                    <a href= game_page.php?game_id='.$result['game_id'].'><h2 id="game_title">'.$result['title'].'&nbsp; &nbsp;</h2></a>
                    <h3 id="platform">for  &nbsp;'.$result['platform'].'</h3>
                </div>
                <p id=game_description>'.$result['description'].'</p>
                <div id="right_side">
                <h4 id="rating">'.$result['rating'].'</h4>
                </div>
                <hr id="hr"/>
            </div>  

        ';
}
        echo '</div>';;?>
    </div>
</div>

`

page three is where i query the database and get results. Not as bad as page 2

function search_results($keywords){
$returned_results = array();
$where = "";

$keywords = preg_split('/[\s]+/', $keywords);
$total_keywords = count($keywords);

foreach($keywords as $key=>$keyword){
    $where .= "title LIKE '%$keyword%'";
    if($key != ($total_keywords - 1)){
        $where .= " AND ";
    }   
}
if (isset($_GET['platform']) && !empty($_GET['platform'])){
    $platform = mysql_real_escape_string(htmlentities(trim($_GET['platform']))); 
        $where .= " AND platform='$platform'";
    }

if (isset($_GET['genre']) && !empty($_GET['genre'])){
    $genre = mysql_real_escape_string(htmlentities(trim($_GET['genre']))); 
                $where .= " AND genre='$genre'";
}

if (isset($_GET['order']) && !empty($_GET['order'])){
    $order = mysql_real_escape_string(htmlentities(trim($_GET['order'])));
    if($where == " ORDER BY $order ASC"){
    $where .= " ORDER BY $order DESC";
    }else{
    $where .= " ORDER BY $order ASC";
    }
}


$results ="SELECT * FROM games WHERE $where ";
echo $results;

$results_num = ($results = mysql_query($results)) ? mysql_num_rows($results) : 0;

if($results_num === 0){
    return false;
}else{
    while($row = mysql_fetch_assoc($results)){
        $returned_results[] = array(
            'game_id' => $row['game_id'],
            'title' => $row['title'],
            'platform' => $row['platform'],
            'rating' => $row['rating'],
            'image_location' => $row['image_location'],
            'description' => $row['description'],
        );
    }
    return $returned_results;
}   
}
?>

Things i need help with

being able to remove a chosen filter category
easier way to pass variables to the query string
better way to structure a filter for mysql
any feedback that will help me improve my code

thanks

David Barker · Accepted Answer · 2012-05-06 17:23:44Z

here's a quick run down on your script from my perspective. Hopefully it will help you out and make your code a bit more efficient and a little prettier! :)

Whilst admirable that you are attempting to stop SQL injection attacks with mysql_real_escape_string() it is really not the best method to protect yourself. Consider instead using prepared statements with PDO, believe me learning this will save you a lot of time!

You also repeat alot of code, for example this line pops up a lot. $keywords = mysql_real_escape_string(htmlentities(trim($_GET[$x])));

Why not put it into a function?

function prepVar($var) {
    return mysql_real_escape_string(htmlentities(trim($var)));
}

and then just use

$platform = prepVar($_GET['platform']);

Another few pointers on isset and empty. You've explicitly asked for a variable to be set and not empty. (check this article out: http://www.htmlcenter.com/blog/empty-and-isset-in-php/)

isset will return TRUE if the variable has a type i.e. String, Object etc. i.e. the variable already exists in the scope.

empty will return TRUE if the variable === 0 || === false || === null

As (in this case) empty will return true if the variable is not set (as the var would === null). You only need to use empty.

$platform = $_GET['platform'];
$platform = !empty($platform) ? prepVar($platform) : '';

The same principles can be applied to your third page with the added complication of adding the $where variable to each of the operations.

if (!empty($platform)) {
    $platform = prepVar($platform);
    $where .= "platform = '{$platform}'";
}

I don't understand this bit of code:

if (isset($_GET['order']) && !empty($_GET['order'])){
    $order = mysql_real_escape_string(htmlentities(trim($_GET['order'])));
    if($where == " ORDER BY $order ASC"){
        $where .= " ORDER BY $order DESC";
    }else{
        $where .= " ORDER BY $order ASC";
    }
}

Are you sure you want to be checking $where for a value, if it contains that value append a second ORDER BY statement to it? That doesn't make much sense :(

Removing a filter category

I would save the search state to $_SESSION each time it is run through your function. You can then recall the previous search state, modify it and then run that through your query again allowing you to remove / change / add as much or as little as you want.

Easier ways to pass to your query string

Firstly you are asking for the same information within your function as you are within the procedural code on your second page.

if (isset($_GET['platform']) && !empty($_GET['platform'])){ ...

As you already have variables that have been mildly sanitised with mysql_real_escape_string() why not pass them to the function as variables as well. Or declare them global variables within the function? In essence the easier way is to make sure you are not duplication your code, setting a variable once and using that variable throughout your script will yield greater stability of your application in the long run.

Better way to structure a filter for mySQL

I dont see a problem with the way you are structuring it, personally I prefer to use functions to decorate SQL queries. In honesty it is each to there own!

Stack Exchange Network

need help improving mysql and php filter code

1 Answer 1

Your Answer

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

Hot Network Questions

need help improving mysql and php filter code

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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