3
\$\begingroup\$

I've written this simple JavaScript application for a homework assignment and I've received feedback saying it could be written better.

Can I get some feedback on how to write this differently? It works for me and returns the results I need, but I would like some more specific feedback. It was meant to be simple and not meant to require more than 2 hours on it.

The requirements were...

  1. You can only edit app.js, you cannot touch index.html. jQuery is provided.
  2. index.html contains two elements: one for the search term and one for the results.
  3. The results should show as a list with each item in an "owner/name" format.
  4. When a result is clicked, display an alert with the repo's language, followers, url and description.
  5. The search term should be cached so duplicate searches do not trigger further requests.
  6. Solution does not need to support older browsers.

Here are the app.js I submitted and the index.html.

(function(window){
	// Setup the variables for the application
	var search,
	results,
	textFieldValue,
	requestBaseUrl = 'https://api.github.com/legacy/repos/search/';
	
	// Enable browser caching for the application	
	$.ajaxSetup({ cache: true});
	
	function applyPageStyling(){
		$( "<style>body { background-image:url(http://n2.9cloud.us/766/a_beautiful_large_scale_shot_for_th_100120255.jpg); font-size:20px; font-family:'Arial, Helvetica', sans-serif;}</style>" ).appendTo( "head" );
		setTimeout(function(){alertInstructions();},1000);
	};
	
	// Alert the user what to do
	function alertInstructions(){
		alert("Please type the term to search GitHub for in the text field and hit the ENTER key.");  
	};
	
	// Function to query the github API
	function gitSearch(){
		// Get the value from the search field
		textFieldValue = $('#search').val();
		
		// Error handling for search box
		if (textFieldValue == ""){
			alert("Please enter a term to search for");
		}
		else {
			// Make the ajax call with the text field value appended to the url. The callback will 
			// output the results or alert the error message
			$("<ul>").appendTo('#results');
			$.getJSON(requestBaseUrl+textFieldValue, function(json, status){})
				// Success then append items to #results div
				.done(function(json){
					$.each(json.repositories, function(i, repositories){
						$("<li>"+json.repositories[i].owner+" / "+json.repositories[i].name+"</li>")
							.appendTo('#results ul')
							// Add a little styling, just a little.
							.css({'cursor': 'pointer','list-style-type':'none', 'width':'500px'})
							.hover(
								function(event) {
									$(this).css({'color':'#ffffff'});
								},
								function(event) {
									$(this).css({'color':'#000000'});
								}
							)
							.click(function(event){
								alert("Repo Lanuage is : "+json.repositories[i].language +"\n"+
								"There are "+ json.repositories[i].followers +" followers of this repo.\n"+
								"The URL of the repo is : "+ json.repositories[i].url +"\n"+
								"The description of the repo : "+ json.repositories[i].description);
							});
					});
			})
			// Log the error message
			.fail(function(status){
				console.log(status);
			});
		}
	};
	
	// Enter key event handler
	$(document).keypress(function(e) {
		if(e.which == 13) {
			gitSearch();
		}
	});
	applyPageStyling();

}(window));
<!DOCTYPE html>
<html lang="en">
	<head>
		<meta http-equiv="Content-type" content="text/html; charset=utf-8">
		<title>Search</title>
		<script src="http://ajax.googleapis.com/ajax/libs/jquery/1/jquery.min.js"></script>
		<script src="app.js"></script>
	</head>
	<body>
		<input id="search" type="text"/>
		<div id="results"></div>
	</body>
</html>

\$\endgroup\$
5
\$\begingroup\$

Looks good to me. You could cache the ul:

var repoList = $("<ul>").appendTo('#results');

So you don´t have to query the dom again when appending the list items. Also it is better to build the list items in memory and only append the complete list to the dom. This way you only trigger one reflow.

So in the $.each loop I would

repoList.append($("<li>somerepo</li>"))

and then outside the loop

$("#results").html(repoList);
\$\endgroup\$
4
\$\begingroup\$

I second user1777136's suggestion of not appending to the DOM in a loop.

Additional suggestions:

  • Get rid of your setTimeout function. Instead, create a $(document).ready handler and prepend your instructions to the body.

  • Change your keypress handler to $('#search').change, which would also go inside $(document).ready. Perform input validation inside the handler to simplify your gitSearch function:

    // Get the value from the search field
    $('#search').change(function(e) {
        if (!$(this).val()) {
            alert("Please enter a term to search for");
        } else {
            gitSearch($(this).val());
        }
    });
    
  • Instead of having individual click listeners defined inside the each loop, store the data for each element and retrieve it with a delegated listener in $(document).ready:

    $('#results').on('click', 'li', function(event) {
         alert("Repo Language is : " + $(this).data('language') + "\n" +
             "There are " + $(this).data('followers') + " followers of this repo.\n" +
             "The URL of the repo is : " + $(this).data('url') + "\n" +
             "The description of the repo : " + $(this).data('description'));
     });
    
  • Keep one string containing all your CSS, including the styles for your li's; you don't need inline CSS for these and this will remove the need for hover listeners. So in addition to your body styles you'd have

    styles += "#results li { cursor: pointer; list-style-type: none; color: #fff; }";
    styles += "#results li:hover { color: #000; }";
    
\$\endgroup\$
  • \$\begingroup\$ Minor notes: you're declaring vars search and results at the top, but I don't think you're using them. Also, when you call gitSearch, set the content of #results to "Loading.." to signal to the user that something is happening in the background. \$\endgroup\$ – VLS Nov 19 '13 at 21:44
  • \$\begingroup\$ I can see how appending to the DOM would increase performance. Thanks for the pointers on that. The other tips make for good performance housekeeping too. Some of the feedback I got regarding the caching, which as far as I can tell is a "coding style" issue is to store the response in an object that uses the search term as a key. The way I coded this was deemed incorrect even though the jquery getJSON is supposed to cache the response even without setting the $.ajaxSetup({ cache: true}); Thoughts? \$\endgroup\$ – DrStomp Nov 19 '13 at 21:49
  • \$\begingroup\$ Turning on caching does cache the response, but still makes an XHR call. Sounds like you need to supersede that by declaring an object (e.g., results = {}), then saving your results in that object (e.g., results[inputText] = json). Then before making your ajax request, see if results[inputText] exists and if it does, use the stored json to create your output. This will require some refactoring, like moving everything in done into a separate function. \$\endgroup\$ – VLS Nov 19 '13 at 22:07
  • \$\begingroup\$ Yeah VLS, that seems to be the method that was the desired outcome. \$\endgroup\$ – DrStomp Nov 19 '13 at 22:16
2
\$\begingroup\$

I have a few other suggestions on how this could be even "better", but after looking it over, it appears another answer here gives some of those suggestions. However, if you'd like me to rewrite the below code using said suggestions, lemme know and I'll update the code.

The suggestions are simple little things like keeping all CSS in one place and discontinuing the hover functions for CSS :hover. As I noted in my comments, I left some of this as was to stay "close" to the train of thought you appeared to be on with your code. While I did introduce some different concepts, I'm hoping the basic flow I tried to stay with will make it easier for you to read through the comments there and see how all these wonderful parts work together!


I would suggest some of the following || this is how I might do it and why:

ExAmPlE

//  My own namespace for any global assignments, thus lowering chance of error of conflict with other code
var mySearch = {
        ajaxSearch: undefined,  //  variable for use with getJSON. useful if you want to stop the ajax call
        current: undefined, //  variable for keeping up with current search term    //   this should also meet your requirements for "cache search"
        //  Method for preforming search, the passing parameter should be the event variable from the action triggering the search
        gitSearch: function(e) {
            //  i don't quite understand not touching the HTML as the input has no name, thus it can't post correctly to server,
            //  tho I see in your example, you simply add the input value to the request url after a "/",
            //  thus I assume the server has a rewrite method for proccessing.
            var query = $(this).val();  //  because of how we call this method, we can simply use "$(this)" to get the input, since this IS the input
            if (mySearch.current == query) return;  //  ensure we're not preforming same search just performed, possibly less than second ago due to multiple ways of calling   //  no need to continue, we just did this work
            mySearch.current = query;   //  reset current search value
            if (!query) alert("Please enter a term to search for");
            else {
                $('#results').html('Searching...'); //  sets content of results area to show it's doing something
                $.getJSON(mySearch.requestBaseUrl + query)
                    .done(function(json) {
                        if (json['repositories']) {
                            if (json.repositories.length) { //  works if return is array (it is) and determines if ANY results were returned
                                //  clear results from previous search and add our new <ul>
                                $('#results').empty().append($('<ul />'));
                                $.each(json.repositories, function(i, v) {
                                    //  variable "v" is same as json.repositories[i]
                                    var txt = v.owner + " / " + v.name,
                                        css = 'cursor: pointer; list-style-type: none; width: 500px;',
                                        sb = [  //  literal array for creating each Line of text
                                            "Repo Lanuage is : " + v.language,
                                            "There are " + v.followers + " followers of this repo.",
                                            "The URL of the repo is : " + v.url,
                                            "The description of the repo : " + v.description
                                        ],
                                        msg = sb.join('\n');    //  this joins our "string builder"/"literal array" into ONE string using "\n" between each "line" (item in array)
                                    //  easy syntax to make new element using jQuery
                                    $('<li />', { text: txt, style: css })
                                        .attr('title', 'Created: ' + new Date(v.created))
                                        .data('msgAlert', msg)  //  assign msg for alert to this elements data for recall
                                        .appendTo($('#results ul'));
                                });
                            }
                            else $('#results').html($('<h2 />', { text: 'No results found.' }));
                        }
                        else alert('Could not find any values.');
                    })
                    .fail(function(status){ $('#results').html('ERROR: Please see console.'); console.log(status); });
            }
        },
        requestBaseUrl: 'https://api.github.com/legacy/repos/search/',
        tmrSearch: undefined    //  this will be used for my "stopped typing" search
    };

$(function() {  //  same as doc.onready /   (function(window)) keeps the window variable closer at hand, but i see no reason for it's use here
    //  add your page styling.
    $('<style />', { html: 'body { background-image:url(http://n2.9cloud.us/766/a_beautiful_large_scale_shot_for_th_100120255.jpg); font-size:20px; font-family:"Arial, Helvetica", sans-serif; }' }).appendTo("head");

    //  since we changed to onready, we can just alert the info, unless you want it to wait a second
    alert("Please type the term to search GitHub for in the text field and hit the ENTER key.");
    //  also, for future reference, when you use setTimeout with a function as you did, you can shortent he call to simply:
    //  setTimeout(alertInstructions, 1000);

    //  As for the calls to begin search, I will go 3 avenues.
    //  First I will include using "Enter" key, but only on input focus
    //  I will also want to preform search on input blur and after user stops typing for a second (like a google quick search)
    //  
    //  I, personally like to delegate my events to $(document), as this makes them readily available for dynamic elements,
    //  however, this can be bad practice. If you attach to many events to the DOM as such, you can end up with overhead that
    //  could crash your program. Since your base example gives us ID's of static elements, I will use those, but I suggest you
    //  read jQuery docs about delegating events. Newer jQuery, as you're using, uses .on -> http://api.jquery.com/on/
    $('#search')    //  the following structure is called "chaining". jQuery provides us this nice way to make multiple calls to one element without recalling that element
    .on('blur', mySearch.gitSearch) // this is all that's needed to register this method to this event // blur is when input loses focus (click on something else)
    .on('keypress', function(e) {
        //  this will apply the Method as if it was the base function of this event
        //  .apply is JS way to trigger a method and send, as params, what to use as "this" and what arguments to feed
        //  note, arguments must be as an array. Doing [e], wraps our Event Argument passed from "keypress" onto the parameter of our method
        if (e.which == 13) mySearch.gitSearch.apply(this, [e]);
    })
    //  the following is one easy way to determine when user stops typing and begin search.
    //  The proccess is simple. Simply clear a timer on keydown, then reset it on keyup. When the timer reaches its end, it preforms the search!
    .on('keydown', function(e) {
        clearTimeout(mySearch.tmrSearch);
    })
    .on('keyup', function(e) {
        var $this = this;   //  needed to pass "this" variable on Timeout
        mySearch.tmrSearch = setTimeout(function() { mySearch.gitSearch.apply($this, [e]); }, 2000);    //  sets to 2 seconds.
        //  if not cleared by keydown within the given time, then "gitSearch" will be preformed!
    })

    //  in finally doing the "gitSearch" method, I noticed you do have a need for delegating events, thus I will delegate those events here
    //  by doing this here, and assinging to document, we don't have to keep rebuilding those event methods everytime you get new search results.
    //  Keep in mind, many, including jQuery recomend using a parent node. However, years of experience with this have taught me, it's not really neccessary
    //  unless you're delegating A LOT of events to the Document. If you have a larger application, find static parents to delegate events too!
    //  Also, I personally am not a big fan of hover. I've had problems in the past with upgrading jQuery and finding unexpected changes on hover methods,
    //  thus I go back to good old mouse-movements
    $(document)
        .on('mouseenter', '#results li', function(e) { $(this).css({'color':'#ffffff'}); })
        .on('mouseleave', '#results li', function(e) { $(this).css({'color':'#000000'}); })
        .on('click', '#results li', function(e) {
            var msg = $(this).data('msgAlert');
            if (msg) alert(msg);
            alert(msg); //  joins our "string builder", literal array into ONE string using "/n" between each "line" (item in array)
        });
})

ExAmPlE


\$\endgroup\$
  • \$\begingroup\$ This looks good, but I stand by a couple of notes in my comment: a. Don't append to the DOM in a loop, create the ul node and append to that and don't append ul to #results till the loop is done. b. No need for mouseenter/mouseleave styles in this case; that can be done in CSS. I would also put everything inside $.each into its own function because that's a lot of indents! \$\endgroup\$ – VLS Nov 22 '13 at 17:31
  • \$\begingroup\$ @VLS point taken, however, in 15 years I've yet to have one problem appending to Dom in a loop and have rarely (cept on long page docs) noticed a difference between that and "createNwait". I thought about the css idea, but figured i'd show the new person some more jQuery usage and explination for. Totally agree with .each to function, but again, trying to keep it readable enough for the OP to go through this line by line and hopefully follow along w/out too much difficulty. \$\endgroup\$ – user10934 Nov 22 '13 at 17:34
  • \$\begingroup\$ @VLS as a matter of fact, if i was 100% to do it "my way", ALL of the CSS would be posted in a <style> tag and pasted to header. Including the individual CSS on the lis \$\endgroup\$ – user10934 Nov 22 '13 at 17:38
  • \$\begingroup\$ Agreed, I had that in my note, just forgot about it. \$\endgroup\$ – VLS Nov 22 '13 at 17:42

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.