3
\$\begingroup\$

I have just completed my first JavaScript/jquery project. It searches customers from .JSON file and sorts them by name or ID. Everything just works fine. The thing is, I am not sure that I have understood all the functions and methods that I have written. Could anyone tell me if this is the best way to read .JSON file and going through the queries that I have written below?

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
    <title>Toolbox Search</title>
    <script src="http://ajax.aspnetcdn.com/ajax/jQuery/jquery-2.0.3.min.js"></script>
    <script src="autocomplete/jquery.easy-autocomplete.min.js"></script>
    <link href="autocomplete/easy-autocomplete.min.css" rel="stylesheet" />
    <link rel="stylesheet" href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css">
    <link href="custom.css" rel="stylesheet" />
</head>
<body>
    <div class="container">
        <div class="jumbotron">
            <div class="col-lg-5">
                <input type="text" id="KUNDE" size="50" placeholder="Search by name." />
            </div>
            <div class="col-lg-2">
                <button class="btn btn-default" type="button" id="buton" value="search" onclick="find();">Search</button>
            </div>
        </div>
        <ul id="list">

        </ul>
        <button id="button"class="btn btn-default">
            Clean Results
        </button>
        <div class="footer"><p> © Copyright 2016</p> <strong><a href=""></a></strong>.</div>
    </div>
    <script> 

        $("#button").click(function (e) {//✔
            $("#list").find('li').remove();
        });

        $(function () {//✔
            $("#KUNDE").focus();
        });
        $(document).keypress(function (e) {//✔
            if (e.which == 13) {
                $("#buton").click();
            }
        });


        var uri = 'json.json'; //The encodeURI() function is used to encode a URI. The differences are only the string elements.
        function find() {       //jQUERY find()Method === The find() method returns descendant elements of the selected element.
            var info = $('#KUNDE').val();//JavaScript variables are containers for storing data values.??????????????
            $.getJSON(uri)//The getJSON() method is used to get JSON data using an AJAX HTTP GET request.
                .done(function (data) { //Method accepts one or more arguments, all of which can be either a single function or an array of functions.?????????????
                    var item = data.filter(function (obj) {//A string containing a selector expression to match the current set of elements against.
                        return obj.name === info || obj.ID === info;//The return statement stops the execution of a function and returns a value from that function.
                    })[0];//???????????????
                    if (typeof item !== 'undefined' || item !== null) {//???????????????
                        $("ul").append("<li>ID      = " + item.ID + "<br />Name    = " + item.name + "<br />Phone      = " + item.phone + "<br />Contact       = " + item.contact + "<br />BalanceLCY      = " + item.balanceLCY + "<br /> CreditLimitLCY       = " + item.creditLimitLCY + "</li>");//✔
                    }
                })
                .fail(function (jqXHR, textStatus, err) {//?????????
                    $('#list').text('Error: ' + err);//??????
                });
        }
        var options = {//✔
            url: "json.json",
            getValue: "name",
            list: {
                match: {
                    enabled: true
                    }
                },
                theme: "square"
            };
            $("#KUNDE").easyAutocomplete(options);
    </script>
</body>
</html>
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to code review, I hope you get some good answers. \$\endgroup\$ – pacmaninbw Sep 9 '16 at 17:26
2
\$\begingroup\$
  • Your code is really hard to read as noted by @Sumurai8. That is the first thing that jumps out at you, which is bad.
  • Consider placing all your javascript code within $(document).ready() handler. There is no reason any of this code needs to operate in global scope.
  • Cache your JSON data. There seems to be no reason to continually call getJSON() if you are calling a static file anyway.
  • Don't keep re-querying the DOM. If you ever have a case where a selector is going to be used more than once during code operations on a page, you should consider storing the jQuery resource in a variable, so as to minimize queries against the DOM. Of course there is is a trade-off of increased memory consumption for this performance gain, but for most applications, that should not be a concern. Particularly good candidates for this approach are those jQuery selectors that happen within event-handler callbacks that get called repeatedly.
  • Your filtering condition in the getJSON() success handler does not seem appropriate to me. I think perhaps the JSON data structure is bad. Why would you need to iterate over the whole array of searchable objects to figure out which ones meets the keyword criteria? Why are you then only working with the first returned result that meets this criteria? The data structure should support your desired lookup case. I would consider building a key-based data structure where you combine the keys from the object id and name values and references to applicable objects against that key. Or alternately have two different data structures - one indexed by id and one indexed by name and make two lookups in response to each find() activity. Either approach would give operational complexity of O(1) in that the find() operation would take a (relatively) fixed amount of time to execute regardless of the size of the data set you are searching against (once data is loaded). Your current approach is O(n), such that operation takes linearly longer to complete as the size of your dataset being searched increases. There is no reason at all to make this method perform so poorly. Between your lack of caching of the data structure and linear complexity, you have a really poorly performing find() function which does all of the following EVERY time the function is called:
    • You re-load JSON (perhaps via HTTP request, perhaps just from browser cache if lucky) - a costly operation where you read a potentially large string into memory.
    • You then deserialize that JSON into a javascript array of objects (again a costly operation for potentially large string)
    • You then iterate every member of that javascript to perform lookup.

You should be striving for a solution where:

  • Either prior to or with first request to find() you retrieve the JSON data source(s), deserialize them and store them in named javascript variables.
  • You have a data structure that allows O(1) lookup against the data set.
  • find() operation uses the stored data structure in javascript to operate against.
\$\endgroup\$
1
\$\begingroup\$

Whitespace

You seem to end at a different indentation than you start. This means the indentation is messed up somewhere. (looking at it, your ending brace for match is incorrect, causing the rest to be indentated wrongly too.)

click and keypress handler

Activating a button with the mouse, touch or a key all fires the click handler as far as I know. Attach the click handler via javascript instead of adding it to the html:

$("#buton").on("click", find);

Don't (deliberatelly) mispell the id of a button, just because it clashes with a different button. If someone decides to correct your spelling mistake, suddenly you have a naming conflict. Instead call them something more descriptive, e.g. btn-clear-results and btn-search.

Comments behind code

Comments behind code make the comments near unreadable. The standard width is 80 columns or so. Having to scroll left and right does not increase the readability of code.

It's better to add comments to logical parts of code than to each line. For example, i = i + 1; //i gets incremented is not useful, because everyone that is familiar with a specific function can tell what is going on. On the other hand, commenting on the larger logical steps that are taken is much more useful.

.val()

The .val() method returns the value of the first element matched by the preceding query. I am not sure if .val() makes sense for non-form element(s).

$.getJSON(..)

$.getJSON(..) is a shorthand for $.ajax(..) with some specific values. It parses the result with $.parseJSON(..) (which may or may not fall through to JSON.parse(..)), which is why you are not working on a string, but instead on native Array's and Objects.

Array.prototype.filter(..)

Array.prototype.filter(..) executes a function on each value of an Array. If the function returns true, the value is kept. If it returns false, the value is discarded. The [0] at the end takes the first value of that array.

If the array is empty, item will be undefined.

.done(..) and .fail(..)

Ajax requests in jQuery are a promise. The .done(..) handler is called when the request was succesful. The .fail(..) handler is called when the request was not succesful. E.g. the url returned a non-2xx or 3xx status code and it could not be completed, or it timed out.

The rest

The rest seems reasonable to me. You use strict comparison. I don't see any variables being dropped into the global namespace, and you consistently close statements with a semicolon. Your style is reasonably constant too.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much I really appreciate your time and effort to help me out. \$\endgroup\$ – Ibrahim İnce Sep 10 '16 at 18:10

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.