2
\$\begingroup\$

I'm using jQueryUI's Autocomplete widget for a product search. The data is in a database, so as the souce I passed it an AJAX request. My problem is, that I need some data about the currently focused option, so in the autocomplete's focus method, I'm calling another AJAX request to the same PHP file, to get some other data.

Here's the jQuery code I'm currently using:

$('.termeknev.ujtermek').autocomplete({
    autoFocus: true,
    minLength: 1,
    source: function (request, response) {
        $.ajax({
            url: "/backend/termekek-query.php",
            type: "POST",
            data: {
                "term": request.term
            },
            success: function (data) {
                response($.map(JSON.parse(data), function (items) {
                    return {
                        value: items.nev,
                        label: items.nev
                    }
                }));
            },
            error: function (error) {}
        });
    },
    focus: function (event, ui) {
        var $elem = event.target,
            $egysegar;
        $.ajax({
            url: "/backend/termekek-query.php",
            type: "POST",
            data: {
                "term": ui.item.value
            },
            success: function (data) {
                $egysegar = data.egysegar;
                $($elem).parent().parent('.ujtermek_wrapper').find('td.egysegar').text($egysegar);
            }
        });

    }
});

And here's the pretty simple PHP file it sends the request to:

<?php
if (isset($_SERVER['HTTP_X_REQUESTED_WITH']) && strtolower($_SERVER['HTTP_X_REQUESTED_WITH']) === 'xmlhttprequest') {
    if(isset($_POST["term"]) && !empty($_POST["term"])) {
        require_once "../includes/constants.php";
        require '../includes/class.db.php';
        $searchTerm = trim($_POST["term"]);

        $db = new db(CONNECTION_STRING, DATABASE_USERNAME, DATABASE_PASSWORD);

        $bind = array(
            ":term" => "%$searchTerm%"
        );

        $results = $db->select(TERMEKEK_TABLE, "nev LIKE :term", $bind, "nev, egysegar");

        echo json_encode($results);
    }
}
?>

Note that I'm also using a PDO wrapper class, but that's irrelevant right now.

Anyway, is there any way to make this cleaner?

\$\endgroup\$
1
\$\begingroup\$

Regarding to readability, the code structure is actually great and it's easy to read the code.

The only thing, regarding to readability, is that you have mixed quotes everywhere.

But still, your code has things you can change:

Javascript:

You are using the autocomplete plugin with a request to the server.

You can skip that whole trouble and simply use the source option.
This will save you a lot of time.

If you don't like it (for whatever reason it is), you should do something on the error event on your $.post call.
Displaying an error message is better than doing nothing.
Ultimately, you can implement a caching system, but this is arguable.

If you still don't want to use the source option, you should remove the var $egysegar.
That var is completely useless.
This line:

$($elem).parent().parent('.ujtermek_wrapper').find('td.egysegar').text($egysegar);

Can be safely replaced by this one:

$($elem).parent().parent('.ujtermek_wrapper').find('td.egysegar').text(data.egysegar);

Of even shorter:

$($elem).parents('.ujtermek_wrapper').find('td.egysegar').text(data.egysegar);

PHP:

On your php file, you have this line:

if(isset($_POST["term"]) && !empty($_POST["term"])) {

According to the php documentation for the function empty():

Returns FALSE if var exists and has a non-empty, non-zero value. Otherwise returns TRUE.
The following things are considered to be empty:
- "" (an empty string)
- 0 (0 as an integer)
- 0.0 (0 as a float)
- "0" (0 as a string)
- NULL
- FALSE
- array() (an empty array)
- $var; (a variable declared, but without a value)

This is a terrible idea!
If the search term is "0", it won't provide any results.
Replace your code with this:

if(isset($_POST["term"]) && $_POST["term"] !== "") {

This will solve the issue by verifying if the term isn't an empty string by using a strict comparison.

Right below, you have this:

require_once "../includes/constants.php";
require '../includes/class.db.php';

You mixed the quotes and you used 2 different instructions to include files.
I assume you can safely use require here, unless you include it on the file class.bd.php.
This will reduce the overhead of checking if the file was included before or not.
More yet (as a sidenote): you can use include instead! (assuming the file will always exist).

On the next line, you have this block:

$searchTerm = trim($_POST["term"]);

$db = new db(CONNECTION_STRING, DATABASE_USERNAME, DATABASE_PASSWORD);

$bind = array(
    ":term" => "%$searchTerm%"
);

$results = $db->select(TERMEKEK_TABLE, "nev LIKE :term", $bind, "nev, egysegar");

echo json_encode($results);

The code itself is fine, but why you store trim($_POST["term"]) in a variable to use it inside a string?
This makes readability a bit hard and it is a terrible idea on this code.
Remove that variable and concatenate it with the "%" string.
This is the result:

$bind = array(
    ":term" => "%".trim($_POST["term"])."%"
);

This is the right way to do it.

Edit (2014/12/01):

Since you are using the LIKE clause, you should escape the % character or it will return funky results.
You should escape it.
A great suggestion can be found here: How to escape literal percent sign when NO_BACKSLASH_ESCAPES option is enabled?
Using that brilliant idea, your code would be like this:

$bind = array(
    ":term" => "%".str_replace('%','|%',trim($_POST["term"]))."%"
);

$results = $db->select(TERMEKEK_TABLE, "nev LIKE :term escape '|' ", $bind, "nev, egysegar");

Or similar.


Outside the scope of the review, you can replace the whole block with only 2 lines:

$db = new db(CONNECTION_STRING, DATABASE_USERNAME, DATABASE_PASSWORD);

echo json_encode( $db->select(TERMEKEK_TABLE, "nev LIKE :term escape '|' ", array( ":term" => "%".str_replace('%','|%',trim($_POST["term"]))."%" ) , "nev, egysegar") );
\$\endgroup\$
  • \$\begingroup\$ Wow, thanks for this throught answer! Amazing quality. Also, thanks for the tips, I've modified my code to reflect what you've written. \$\endgroup\$ – PeterInvincible Dec 1 '14 at 12:52
  • \$\begingroup\$ @TrisztánThar You are welcome. That list (for the empty() function) gave me a lot of work. I know it's a tiny detail, but still. I've edited my answer regarding an important issue that could happen when the term has the character %. \$\endgroup\$ – Ismael Miguel Dec 1 '14 at 16:12

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.