2
\$\begingroup\$

I want to improve my code or structure. How does it look? Any comments or suggestions?

$(function() {

    function searchGit() {
        //grab value of search field
        var search = $('#search').val();

        // Validates entered value
        if (search.length > 0) {
            //pull json data from url
            $.ajax({
                url: 'https://api.github.com/legacy/repos/search/' + search,
                dataType: 'json',
                success: function(data) {

                    // use the results to template the html using _
                    var results = data.repositories,
                        resultsTemplate = _.template($("#results-template").html()),
                        resultingHtml = resultsTemplate({results : results});

                    // place the generated data into the html
                    $("#results-container").html(resultingHtml);
                }
            });
        } else {
            alert("Please enter repo name");
        }

        return false;
    }

    $('#submit').on('click', searchGit);
});
\$\endgroup\$
2
\$\begingroup\$

I would only change a few things around. First, this is all one big monolithic function. Break it apart into smaller individual functions and try to have each function do one job only.

Second, I would convert from using the success callbacks to using Promises. That might be a matter of taste; Feel free to ignore. Third, I would encapsulate your code inside an IIFE to create a private scope. It also insures that $ is going to be jQuery.

Next, I would move all of the individual functions out of the document.ready event. There is no reason to wait until then to load the functions. Lastly, I would change to using the submit event instead of the click event. I would guess that is really when you need to do your call.

(function( $ ) {
  // IIFE for a private scope

  function getData( search ) {
    return  $.ajax({
       url: 'https://api.github.com/legacy/repos/search/' + search,
       dataType: 'json'
    });
  }

  function searchGit( event ) {
    //grab value of search field
    event.preventDefault();
    var search = $('#search').val();
    // Validates entered value
    if ( search.length ) {
      $.when( getData( search ) ).done( function( data ) {
        processData( data );
      });
      /* could also be written like this:
         var call = getData( search );
         call.done( function( data ) { ... });
                  OR
         getData( search ).done( function( data ) { ...});
      */
    } else {
      alert( "Please enter repo name" );
    }
    return false;
  }

  function processData( data ) {
    // use the results to template the html using _
    var results = data.repositories,
        resultsTemplate = _.template($("#results-template").html()),
        resultingHtml = resultsTemplate({results : results});

    // place the generated data into the html
    $("#results-container").html( resultingHtml );
  }

  $(function(){
    $('#submit').on( 'submit', searchGit );
  });

})( jQuery );

Eventually, I would recommend replacing all of the DOM selectors with variables. That way it would be less coupled together. For example, in the process data function:

  function processData( data , templateName, resultsElement ) {
    // use the results to template the html using _
    var results = data.repositories,
        resultsTemplate = _.template( $( templateName ).html() ),
        resultingHtml = resultsTemplate( {results : results} );
    // place the generated data into the html
    $( resultsElement ).html( resultingHtml );
  }

Hope that helps.

\$\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.