1
\$\begingroup\$

A friend helped me put the following code together, which is made up of a series of repetitive loadContent() functions. I've just shown the first two functions but they're all the same.

The code works fine but I'm wondering if anyone could give me a tip as to how I could reduce the bloat in this code and condense the functions somehow. I read somewhere that repetitive code usually means that the code can be simplified or shortened somehow. I'm no expert at jQuery or coding so I'm not sure where to start. I'm assuming that the code could be written a lot better but I might be wrong.

function loadContent1()
    {
            $('#content1').empty();
            if(typeof(counterArray[1]) != 'undefined' && counterArray[1] !=     null)clearTimeout(counterArray[1]);
            jQuery.ajax({
            type: "GET",
            url: "//cgi-bin/mggsn_ssr21.cgi",
            beforeSend:function(){
                    jQuery('#content1').append('<img class="loading"     src="img/loading.gif" alt="Loading..." />');
                    jQuery('#refresh1text').html("<p>Loading...</p>");
            }
            })
            .done(function(data){
                    if(typeof(counterArray[1]) != 'undefined' && counterArray[1] != null)clearTimeout(counterArray[1]);
                    $('#content1').empty();
                    $('#content1').html(data);
                    //alert(data.slice(0.100));

                    countDownTimeArray[1]=countDownIntervalArray[1];
                    countDown(1);
            })
                    .fail(function(){
                    $('#content1').empty();
                    jQuery('#content1').html('<h4>failed to load content</h4>' + this.url);
            });
    }

    function loadContent2()
    {
            $('#content2').empty();
            if(typeof(counterArray[2]) != 'undefined' && counterArray[2] != null)clearTimeout(counterArray[2]);
            jQuery.ajax({
            type: "GET",
            url: "/cgi-bin/mtest2.cgi",
            beforeSend:function(){
                    jQuery('#content2').append('<img class="loading" src="img/loading.gif" alt="Loading..." />');
                    jQuery('#refresh2text').html("<p>Loading...</p>");
            }
            })
            .done(function(data){
                    if(typeof(counterArray[2]) != 'undefined' && counterArray[2] != null)clearTimeout(counterArray[2]);
                    $('#content2').empty();
                    $('#content2').html(data);
                    //alert(data.slice(0.100));

                    countDownTimeArray[1]=countDownIntervalArray[1];
                    countDown(2);
            })
                    .fail(function(){
                    $('#content2').empty();
                    jQuery('#content2').html('<h4>failed to load content</h4>' + this.url);
            });
    }
\$\endgroup\$
1
\$\begingroup\$

You could simplify like so:

For the HTML, you could embed specifics as data-* attributes, which you can fetch in JS later on.

<div class="content" data-source="//cgi-bin/mggsn_ssr21.cgi" data-refreshtext="#refresh1text'>
  ...
</div>

<div class="content" data-source="/cgi-bin/mtest2.cgi" data-refreshtext="#refresh2text'>
  ...
</div>

As for the loading gif, I'm not a fan of <img>. I'm more into making the loading gif a background that's coupled with a class, so that it's easy to add using addClass and remove using removeClass.

.content.loading{
  background-image: url(img/loading.gif);
  background-repeat: no-repeat;
  background-position: center center;
  min-height: 100px;
}

As for the function, we grab the data-* attributes, and execute the ajax.

function loadContent(selector){
  var element = $(selector);
  var url = element.data('source');
  var refreshSelector = element.data('refreshtext');

  $.ajax({
    type: "GET",
    url: url,
    beforeSend: function () {
      // Empty the element and set it to loading
      element.empty().addClass('loading');
      $(refreshSelector).html("<p>Loading...</p>");
    }
  }).done(function(data){

    element.html(data);
    ...

  }).fail(function(){

    element.html('<h4>failed to load content</h4>' + this.url);
    ...

  }).complete(function(){
    // for all cases, fail or success, remove the gif background
    element.removeClass('loading');
  });

}

// use like:
loadContent('.content');

You may supply any selector as your parameter here, like an ID. Just make sure it has the right data-* attributes to go with it.

\$\endgroup\$
2
\$\begingroup\$

Joseph cleaned up the code a lot so let me point out a few things that you'll be able to use in your future code.

  • Consistent formatting greatly improves code readability and maintenance. There are many out there that are good starts, and I'm a fan of Google's language styles for the most part.

  • Since you're already depending on the definition of $, it's odd to mix it with jQuery in the same code. Pick one and stick with it.

  • To check that a value is neither null or undefined you can simply compare it to null using ==/!=. When you know the value cannot be some other falsy value (e.g. false, 0, or ''), you can simply use ! or the boolean context.

    if (counterArray[1]) { ... }
    
  • DOM queries can be expensive on large pages, so it pays to save the results of a query if you're going to use it multiple times, and it cleans up the code. I like prefixing the name of a variable holding the results of a selector with a $ to make it clear that it's not a raw DOM element.

    var $content = $('#content1');
    
  • Most jQuery methods return the selector itself, allowing you to chain multiple calls together.

    $content.empty().html(data);
    
  • html completely replaces the contents of the element so there's no reason to call empty on it first. DOM manipulation causes a reflow; combine your manipulations when possible.

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