2
\$\begingroup\$

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.

my database table looks like this game_id |title |developer |publisher |genre |release_date |platform

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

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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!

\$\endgroup\$

Your Answer

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

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