2
\$\begingroup\$

The following search script creates a JSON response for an autocomplete field. The tables and fields search from are flexible to allow using the same script for different type of searches - the idea is to search from each database field separately and see how often a term appears and sort them in descending order.

The part about ordering the found hits for a word by number of occurrences seems like it could be improved, but I don't know how to better approach the problem.

if (isset($_REQUEST['term']) && !empty($_REQUEST['term'])) {
    
    // define what table -> array(fields) to search from based on a possibly sent
    // parameter 'type', which is 'customer' or 'admin' or empty or some specific
    if (!isset($_REQUEST['type']) || $_REQUEST['type'] === 'customer') {
        $searchFrom = array( 
            'tuotesarjat' => array('sarjanimi', 'laattatyyppi'),
            'tuotteet'    => array('varikategoria1', 'varikategoria2', 'pinta', 'hakupinta')
        );
    } else if ($_REQUEST['type'] === 'admin') {
        $searchFrom = array( 
            'tuotesarjat' => array('sarjanimi', 'laattatyyppi'),
            'tuotteet'    => array('tuotenumero', 'kuvaus1', 'varikategoria1', 'varikategoria2', 'pinta', 'hakupinta')
        );
    } else if ($_REQUEST['type'] === 'tuotesarja') {
        $searchFrom = array(
            'tuotesarjat' => array('sarjanimi')
        );
    } else if ($_REQUEST['type'] === 'tuotenumero') {
        $searchFrom = array(
            'tuotteet'    => array('tuotenumero')
        );
    }
    
    $found = array();
    $terms = explode(' ', $_REQUEST['term']);

    // loop through all split up terms
    foreach ($terms as $key => $term) {
        if (!empty($term)) {
            $term = mysql_real_escape_string($term);
            
            // loop through tables
            foreach ($searchFrom as $table => $cols) {
                // for searches from table 'tuotesarjat' limit hits to published items
                $andWhere = $table == 'tuotesarjat' ? ' AND arkistoitu != 1 ' : '';
                
                // loop through fields
                foreach ($cols as $column) {
                    $sql = "SELECT $column FROM $table WHERE $column ";
                        
                    // to limit searches, for 1-2 letter terms search string start, 
                    // else from whole term
                    if (strlen($term) < 2) {
                        $sql .= "LIKE '$term%' $andWhere GROUP BY $column";
                    } else {
                        $sql .= "LIKE '%$term%' $andWhere GROUP BY $column";
                    }
                    // in case a field does not exist, fail silently 
                    // (a json query will still be generated returning the 
                    // _other_ fields' searches' results)
                    $res = @mysql_query($sql); 
                    if (mysql_num_rows($res) > 0) {
                        while($row = mysql_fetch_assoc($res)) {
                            // insert new found word to array with count 1
                            // or increment already found words' counts
                            if (!key_exists($row[$column], $found)) {
                                $found[$row[$column]] = 1;
                            } else {
                                $found[$row[$column]] += 1;
                            }
                        }
                    }
                }  
            }          
        }
    }
    
    // sort by number of hits for each word desc
    arsort($found);
    
    // filter empty strings
    $found = array_filter(array_keys($found));
    
    // reduce array to global max hits
    while (count($found) > MAX_LIVESEARCH_SUGGESTIONS) {
        array_pop($found);
    }
    
    // generate and echo json
    $json = '[';
    foreach ($found as $sana) {
        $json .= '"' . $sana . '", ';
    }
    $json = substr($json, 0, -2) . ']';
    echo $json;    
}
\$\endgroup\$
2
\$\begingroup\$

Upgrading PHP

Upgrade your PHP version. After reading this code I determined that you are running at the most 4.0.6. key_exists() was replaced with array_key_exists() in 4.1.1, and this was some time ago. An old version of PHP runs the risk of having security issues, and you won't be able to get much help because everyone is using the newer version. Some of these suggestions might still work, but others may not be supported in your version, such as this first one.

REQUEST vs. POST/GET

You should stop using $_REQUEST. It is insecure, being that you are running an older version of PHP this is understandable, but if you upgrade, you should definitely update this as well. If you know where the information is coming from, and you should, then you should just use that array. For example, if it is coming from POST use the $_POST array, if it is coming from GET use the $_GET array, etc...

switch vs. if/else

Think of using a switch. It is faster than if/else statements and, in my opinion, cleaner looking and more legible.

$searchFrom = array(
    'tuotesarjat' => array('sarjanimi', 'laattatyyppi'),
    'tuotteet'    => array('varikategoria1', 'varikategoria2', 'pinta', 'hakupinta')
);

if( isset( $_REQUEST[ 'type' ] ) {
    $type = $_REQUEST[ 'type' ];
    switch( $type ) {
        case 'admin':
            $searchFrom = array( 
                'tuotesarjat' => array('sarjanimi', 'laattatyyppi'),
                'tuotteet'    => array('tuotenumero', 'kuvaus1', 'varikategoria1', 'varikategoria2', 'pinta', 'hakupinta')
            );
        break;
        //Use a case for each "type", exclude customer as it was defined above as a default
    }
}

Foreach Keys

No need to return $key in your foreach loop, especially since you're not using it. Just remove $key => from the declaration. You can also just use array_filter() after exploding it into an array to remove any empty array members. This will relieve some overhead and is cleaner. Also, since you probably don't want to search for the same term twice you can use array_unique() to remove duplicates.

Repetition

Try to make it so that you repeat yourself as little as possible. This is so that if you ever need to change something, you'll only have to do so once. So...

$term .= '%';
if (strlen($term) > 2) { $term = "%$term"; }

$sql .= "LIKE '$term' $andWhere GROUP BY $column";

You'll also find, if you upgrade your PHP version, that mysql() is being deprecated. mysqli() and pdo() are now the preferred methods.

Error Suppressors

DON'T USE ERROR SUPPRESSORS. This is a sign of bad code. If there is an error fix it. If you are doing it just so you don't have to type a couple extra lines, don't. Always ensure your code works 100%. The error suppressor should only be used during debugging.

Incrementing

There's got to be some better way to increment the "found" array... I thought of using a ternary operator, but that got a little messy, and at the moment, I can't think of anything else... I'll update this if I think of anything...

Question

How are there empty strings in $found?

MAX_LIVESEARCH_SUGGESTIONS

Not sure what the MAX_LIVESEARCH_SUGGESTIONS is, but would it not make more sense to perform this before creating the $found array?

$length = count( $terms );
while ( $length > MAX_LIVESEARCH_SUGGESTIONS) { array_pop( $terms ); }

Notice I moved count() out of the loop. With it in the loop, it will run it every time, which means more overhead. Remove it from the loop to remove the overhead. I'm having difficulties following the program, so unless $found ends up with more elements than $terms the above should work. If there are more, then you should limit how many are recorded by breaking out of the loops once that limit is reached.

$length = count( $found );
if( $length == MAX_LIVESEARCH_SUGGESTIONS ) { break 4; } //I think its 4... might be 3...
//perform actions to set $found here

JSON

Updating your PHP will also mean that you can use the JSON library that is now standard instead of needing to recreate it.

Hope this helps!

\$\endgroup\$
5
  • \$\begingroup\$ Thanks so much for the input. Many of your points are very good and I will implement them or see how I can restructure my code. As for my use of $_REQUEST I should point out that the script is called both via GET and via POST (as a fallback, in case ajax fails/javascript is turned off), so I though $_REQUEST will cover both scenarios. The error suppressing @ is silly and I should probably just make sure the fields I search from exist. Not sure if you suggestion for the while loop will work. I have x amount of words to suggest for a term, but might want to limit the amount of suggestions. \$\endgroup\$ – kontur Jul 2 '12 at 17:07
  • \$\begingroup\$ Also, you are obviously right about the version of PHP, but that is, unfortunately, beyond my control. \$\endgroup\$ – kontur Jul 2 '12 at 17:08
  • \$\begingroup\$ @kontur: GET, POST, and COOKIES are for vastly different data types. As such they are stored separately. So using REQUEST, which combines these arrays, will override information from the others. This means that it is very easy for a user to forge information that you don't want them to have access to. So it is much better to know explicitly where that information is coming from and limit access to it from that array only. While this does not appear to be an issue here, it is still good to be in the habit of using the appropriate array. \$\endgroup\$ – mseancole Jul 2 '12 at 18:41
  • \$\begingroup\$ I'd bug whoever you need to to get your PHP updated. If its not possible, then its not possible. But if you, or whoever you are working for, is paying for a server that only has this version I would seriously contemplate switching hosts. If its compatibility issues that are holding you back, know that PHP is pretty backwards compatible. Those things that have been deprecated are usually replaced with similar functions, so a find/replace script shouldn't be too difficult. \$\endgroup\$ – mseancole Jul 2 '12 at 18:42
  • \$\begingroup\$ Thanks for the input @showerhead. We are in the progress of moving to a new host, but things are slow :) I did not know $_REQUEST also holds $_COOKIES. As you rightly pointed out, specifically working with either or might also help to improve the readability of the script. Thanks again. \$\endgroup\$ – kontur Jul 2 '12 at 20:16
2
\$\begingroup\$
  1. isset($variable) && !empty($variable) is a classic/common anti-pattern that should not exist in anyone's code. Please read this: https://stackoverflow.com/a/4559976/2943403

  2. A lookup array will allow you to ditch all those condition blocks.

    $lookup = [
        'customer' => [
            'tuotesarjat' => ['sarjanimi', 'laattatyyppi'],
            'tuotteet'    => ['varikategoria1', 'varikategoria2', 'pinta', 'hakupinta']
        ],
        'admin' => [
            'tuotesarjat' => ['sarjanimi', 'laattatyyppi'],
            'tuotteet'    => ['tuotenumero', 'kuvaus1', 'varikategoria1', 'varikategoria2', 'pinta', 'hakupinta']
        ],
        ...
    ];
    
  3. !isset($_REQUEST['type']) || $_REQUEST['type'] === 'customer' means that you can fallback to customer when the element is not declare or is null. Now $searchFrom can be populated in clean O(1) action. In modern php, the null coalescing operator makes this syntax sweeter:

    $searchFrom = $lookup[$_REQUEST['type'] ?? 'customer'] ?? $lookup['customer']; 
    

    For PHP version that are too old to enjoy the null coalescing operator:

    $searchFrom = isset($_REQUEST['type'], $lookup[$_REQUEST['type']]) ? $lookup[$_REQUEST['type']] : $lookup['customer']; 
    
  4. In fact, if those lookup values are mostly static, then I would argue that you should refactor the script so that the lookup contains sql string with placeholders. This will set you up to use a single , non-iterative prepared statement. However, if your queries are prohibitively dynamic to consider a single prepared statement then try to create a prepared statement for each table. Prepared statements that contain a variable number conditions are not impossible to make (notice that the result set object can be iterated with a foreach() instead of iterated fetch() calls). In modern PHP, it no longer best practice to use the deprecated mysql_ functions or escapeing functions.

  5. You must not manually craft json strings. When you want to create a json string in PHP, always use json_encode() so that nothing can go wrong.

  6. An easy fix that be consistent throughout your php applications is that else if should be one word (elseif) to comply with PSR-12 coding standards.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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