Functions that increment count variables and progress bars on button click

Question

I'm just learning JavaScript and jQuery, and I've got some code that works, but I know it can be optimized. I have a page that when a button is clicked it adds 1 to a label, and then increments a bootstrap progress bar by 1. I have 6 functions that could probably be reduced to 2 by passing in arguments instead of having 6 hard coded functions.

Would it be possible to reduce these 6 down to just 1? Also, am I using the if/else blocks right? Or is there a better way to do that?

Again, I am just learning, but I want to learn the right techniques and formats.

Here is a gist if that is easier.
Here is the jsfiddle as well.

$(document).ready(function(){
    $("#button1").click(addVisit);
    $("#button2").click(removeVisit);
    $("#button3").click(addVisit2);
    $("#button4").click(removeVisit2);
    $("#button5").click(addVisit3);
    $("#button6").click(removeVisit3);
});

function addVisit(){
    var countTag = $('#count');
    var count = countTag.text();
    var progressTag = $('#percent');
    count ++;
    if (count <= 4){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-success');
    } else if (count > 4 && count < 8){
        progressTag.removeClass('progress-bar-success');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-warning');
    } else if (count >= 8){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-success');
        progressTag.addClass('progress-bar-danger');
    }
    countTag.text(count);
    progressTag.css('width', count * 10 + "%");
}

function removeVisit(){
    var countTag = $('#count');
    var count = countTag.text();
    var progressTag = $('#percent');
    count --;
    if (count <= 4){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-success');
    } else if (count > 4 && count < 8){
        progressTag.removeClass('progress-bar-success');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-warning');
    } else if (count >= 8){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-success');
        progressTag.addClass('progress-bar-danger');
    }
    countTag.text(count);
    progressTag.css('width', count * 10 + "%");
}

function addVisit2(){
    var countTag = $('#count2');
    var count = countTag.text();
    var progressTag = $('#percent2');
    count ++;
    if (count <= 4){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-success');
    } else if (count > 4 && count < 8){
        progressTag.removeClass('progress-bar-success');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-warning');
    } else if (count >= 8){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-success');
        progressTag.addClass('progress-bar-danger');
    }
    countTag.text(count);
    progressTag.css('width', count * 10 + "%");
}

function removeVisit2(){
    var countTag = $('#count2');
    var count = countTag.text();
    var progressTag = $('#percent2');
    count --;
    if (count <= 4){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-success');
    } else if (count > 4 && count < 8){
        progressTag.removeClass('progress-bar-success');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-warning');
    } else if (count >= 8){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-success');
        progressTag.addClass('progress-bar-danger');
    }
    countTag.text(count);
    progressTag.css('width', count * 10 + "%");
}

function addVisit3(){
    var countTag = $('#count3');
    var count = countTag.text();
    var progressTag = $('#percent3');
    count ++;
    if (count <= 4){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-success');
    } else if (count > 4 && count < 8){
        progressTag.removeClass('progress-bar-success');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-warning');
    } else if (count >= 8){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-success');
        progressTag.addClass('progress-bar-danger');
    }
    countTag.text(count);
    progressTag.css('width', count * 10 + "%");
}

function removeVisit3(){
    var countTag = $('#count3');
    var count = countTag.text();
    var progressTag = $('#percent3');
    count --;
    if (count <= 4){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-success');
    } else if (count > 4 && count < 8){
        progressTag.removeClass('progress-bar-success');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-warning');
    } else if (count >= 8){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-success');
        progressTag.addClass('progress-bar-danger');
    }
    countTag.text(count);
    progressTag.css('width', count * 10 + "%");
}

Welcome to Code Review! As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. For examples of good titles, check out Best of Code Review 2014 - Best Question Title Category You may also want to read How to get the best value out of Code Review - Asking Questions. — Quill, Commented Dec 30, 2015 at 14:12
It appears that your code is not working as intended and therefore would not be on-topic for Code Review until it does. Please see What topics can I ask about here? for more details. — Phrancis, Commented Dec 30, 2015 at 14:24
My apologies, I misunderstood the title suggestions. @Phrancis, my code does work, with the hard coded functions. See the jsfiddle I added. — Greyhammer, Commented Dec 30, 2015 at 14:26
Please do not change your code after answers have been posted, in order not to invalidate them. See What should I do when someone answers my question?. — Phrancis, Commented Dec 30, 2015 at 18:34
@Phrancis OK, Thanks for the advise. Here is my updated code, for reference. jsfiddle — Greyhammer, Commented Dec 30, 2015 at 18:47

user5325596 · Accepted Answer · 2015-12-30 17:04:09Z

You could create a function called updateProgress and pass the selector of the progress bar and count to it.

function updateProgress (selector, count) {
    var progressTag = $(selector); 

    if (count <= 4){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-success');
    } else if (count > 4 && count < 8){
        progressTag.removeClass('progress-bar-success');
        progressTag.removeClass('progress-bar-danger');
        progressTag.addClass('progress-bar-warning');
    } else if (count >= 8){
        progressTag.removeClass('progress-bar-warning');
        progressTag.removeClass('progress-bar-success');
        progressTag.addClass('progress-bar-danger');
    }
     progressTag.css('width', count * 10 + '%');
}

which can be further simplified to

function updateProgress (selector, count) {
    var progressTag = $(selector); 

    progressTag.toggleClass('progress-bar-success', count <=4);
    progressTag.toggleClass('progress-bar-warning', count > 4 && count < 8);
    progressTag.toggleClass('progress-bar-danger', count >= 8);

    progressTag.css('width', count * 10 + '%');
}

You can also have one addVisit method, taking an id as parameter.

$(document).ready(function(){
    $("#button1").click(addVisit.bind(null, ""));
    $("#button3").click(addVisit.bind(null, "2"));
    $("#button5").click(addVisit.bind(null, "3"));
    // TODO : use the same approach for removeVisit
    $("#button2").click(removeVisit);
    $("#button4").click(removeVisit2);
    $("#button6").click(removeVisit3);
});

function addVisit (id) {

    var countTag = $('#count' + id);
    var count = countTag.text();
    var progressTag = $('#percent' + id);
    // or updateProgress('#percent' + id); 
    //etc
}

Ideally though, you would like to have something like

$(document).ready(function(){
     $(".add-btn").click(addVisit);
     $(".remove-btn").click(removeVisit);
 });

so that if you need to add another set of buttons and progress to your html the javascript is aleady taken care of.

<h1>David Visits&nbsp;&nbsp;<span class="label label-default" id="count-david">0</span></h1>
    <div class="progress">
        <div id="percent-david" class="progress-bar" role="progressbar" style="width: 0"></div>
    </div>
    <button data-id="david" class="btn add-btn">Add Visit</button>
    <button data-id="david" class="btn remove-btn">Remove Visit</button>

So you would need your buttons to have class of add-btn and remove-btn, the count and progress to have ids that can be taken from an attribute of the buttons e.g count-david, percent-david, and then addVisit might look something like

 function addVisit () {
     var btn = $(this);
     var id = btn.attr("data-id"); 

     var countTag = $('#count-' + id); 
     var count = countTag.text();
     var progressTag = $('#percent-' + id);
     // or updateProgress('#percent-' + id); 
     // etc


 }

Community · Accepted Answer · 2020-06-10 13:24:26Z

Styling and readability

While javascript allows to define strings with single quotes and double quotes, I strongly recommend to use one type, as both types of strings are functionally equivalent. Simply escape single quotes or double quotes based on which type of string you decide to use. Mixing them, such as you did in the following code, will lead to confusing situations where it is unclear where a string starts or ends.

progressTag.css('width', count * 10 + "%");

Reducing the number of functions does not increase the readability of code. Just because you can condense the code down to one function does not mean you have to do this, or that it does make the code more readable. It is more important to make sure that a function does only do one task.

Code duplication

Your code has a lot of code duplication. The most obvious case you already pointed out: addVisit, addVisit2 and addVisit3 are identical except for the identifiers. You could make it a function addVisit(countId, progressId). The click handlers would need to be adjusted.

You partly duplicate the removal of classes. You could push this to the top and only add classes in the if-statement:

progressTag.removeClass('progress-bar-success');
progressTag.removeClass('progress-bar-warning');
progressTag.removeClass('progress-bar-danger');
if (count <= 4){
    progressTag.addClass('progress-bar-success');
} else if (count > 4 && count < 8){
    progressTag.addClass('progress-bar-warning');
} else if (count >= 8){
    progressTag.addClass('progress-bar-danger');
}

Speaking of which, that part of the code is identical for both addVisit and removeVisit. On closer inspection, your addVisit and removeVisit functions do two things. They modify the count, then they update the progress bar. You could split that in two functions. Think about what you would need to duplicate in case that you, for example, want to add a reset button.

Logic errors

Your addVisit and removeVisit functions do not guard against underflow and overflow. It is perfectly fine to have -3 visits in your implementation.

mlnmln · Accepted Answer · 2015-12-30 17:34:25Z

I rewrote your script.

https://jsfiddle.net/mlnmln/6c3ky4ff/

HTML

<body>
<div id="content" class="col-md-4">

    <div class="js-Visit">
      <h1>David Visits&nbsp;&nbsp;<span class="label label-default js-Visit-count">0</span></h1>
      <div class="progress">
          <div class="progress-bar js-Visit-progressBar" role="progressbar" style="width: 0"></div>
      </div>
      <button class="btn js-Visit-add">Add Visit</button>
      <button class="btn js-Visit-remove">Remove Visit</button>      
    </div>

    <div class="js-Visit">
      <h1>Joey Visits&nbsp;&nbsp;<span class="label label-default js-Visit-count">0</span></h1>
      <div class="progress">
          <div class="progress-bar js-Visit-progressBar" role="progressbar" style="width: 0"></div>
      </div>
      <button class="btn js-Visit-add">Add Visit</button>
      <button class="btn js-Visit-remove">Remove Visit</button>      
    </div>

    <div class="js-Visit">
      <h1>Times CS Leaves Early for no Reason&nbsp;&nbsp;<span class="label label-default js-Visit-count">0</span></h1>
      <div class="progress">
          <div class="progress-bar js-Visit-progressBar" role="progressbar" style="width: 0"></div>
      </div>
      <button class="btn js-Visit-add">Add Early Leave</button>
      <button class="btn js-Visit-remove">Remove Early Leave</button>      
    </div>
</div>
</body>

JavaScript

(function($, window, document, undefined) {

  var defaults = {
    min: 0, 
    max: 10, 
    count: 0    
  };

  function Visit(element, options) {
    this.element = element;            
    this.options = $.extend( {}, defaults, options) ;      
    this.view = {
      count: this.element.find('.js-Visit-count'), 
      progressBar: this.element.find('.js-Visit-progressBar')
    };
    this.init();
  };

  Visit.prototype = {

    init: function() {      
      this.element.on('click', '.js-Visit-add', $.proxy(this.add, this));
      this.element.on('click', '.js-Visit-remove', $.proxy(this.remove, this));      
    }, 

    setOption: function(key, value) {
      if (key === 'count') {
        value = Math.max(this.options.min, Math.min(this.options.max, value));
      }
      this.options[key] = value;
      this.render();
    }, 

    add: function() {
      this.setOption('count', this.options.count + 1);
    }, 

    remove: function() {
      this.setOption('count', this.options.count - 1);      
    }, 

    render: function() {
      this.view.count.text(this.options.count);
      this.view.progressBar.css('width', this.options.count / this.options.max * 100 + '%');        
      this.states = [
        {
          test: this.options.count <= 4, 
          className: 'progress-bar-success'
        }, 
        {
          test: this.options.count > 4 && this.options.count < 8, 
          className: 'progress-bar-warning'
        }, 
        {
          test: this.options.count >= 8, 
          className: 'progress-bar-danger'
        }
      ];      

      for (var i = 0; i < this.states.length; i++) {
        var state = this.states[i];          
        this.view.progressBar.toggleClass(state.className, state.test);
      } 
    }    
  };


  $(function() {    
    $('.js-Visit').each(function() {
        new Visit($(this));    
    });
  });

})(jQuery, window, document);

Major Concerns

Code duplication

addVisit, addVisit2, addVisit3 are essentially the same, but with different identifiers. This is a clear sign that they could be condensed into one function which accepts parameters.

Global scope pollution

Your are polluting the global variable scope. By default all variables live on the global window object. Because of possible naming conflicts with other libraries this is considered bad practice. An Immediately-Invoked Function Expression (IIFE) protects against that.

(function($, window, document, undefined) { 
  // ... 
})(jQuery, window, document);

http://benalman.com/news/2010/11/immediately-invoked-function-expression/

Missing overflow/underflow protection

Your counter is not protected against underflow and overflow. This can easily be fixed using Math.min and Math.max.

value = Math.max(this.options.min, Math.min(this.options.max, value));

Minor Concerns

$(document).ready(function(){ ... }) can be written as $(function() { ... })

PS: I did a complete rewrite and I don't have the time to explain everything I did now, but I will gladly answer upcoming questions.

Stack Exchange Network

Functions that increment count variables and progress bars on button click

3 Answers 3

Styling and readability

Code duplication

Logic errors

Major Concerns

Code duplication

Global scope pollution

Missing overflow/underflow protection

Minor Concerns

Your Answer

Not the answer you're looking for? Browse other questions tagged
javascript
jquery
or ask your own question.

Hot Network Questions

Functions that increment count variables and progress bars on button click

3 Answers 3

Styling and readability

Code duplication

Logic errors

Major Concerns

Code duplication

Global scope pollution

Missing overflow/underflow protection

Minor Concerns

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javascriptjquery or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
javascript
jquery
or ask your own question.