3
\$\begingroup\$

I'm writing simple jQuery script for updating one field on another field's change. It's working, but I'm trying to write good code and use best practices. Can you please tell me what my mistakes and 'bad' things in this code?

Maybe I should use getTotalUserScore() in updateTotalUserScore(), not in $('.js-question-score').change() function, or I shouldn't use 2 functions for so simple piece of logic, or my variables names are bad. Tell me anything you feel about this code. Also, I've realized that my total-score field sometimes updates with delay.

$('.js-question-score').change(function () {
    var totalScore = getTotalUserScore();
    updateTotalUserScore(totalScore);
});

function updateTotalUserScore(totalScore) {
    $('.js-total-user-score').text(totalScore)
}

function getTotalUserScore() {
    var totalScore = 0;
    $('.js-question-score').each(function() {
        totalScore += parseInt($(this).val());
    });
    return totalScore;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<form> 
  <input class="js-question-score" name="first" min="0" max="20"  value="0" type="number"/>
  <input class="js-question-score" name="second" min="0" max="20"  value="0" type="number"/>
  <input class="js-question-score" name="third" min="0" max="20"  value="0" type="number"/>
</form>

<div>
  User total score:
  <span class="js-total-user-score">
    0
  </span>
</div>

\$\endgroup\$

migrated from stackoverflow.com Oct 13 '16 at 18:53

This question came from our site for professional and enthusiast programmers.

  • 3
    \$\begingroup\$ best practice is don't repeat yourself (DRY) so chache the variable \$\endgroup\$ – EaBangalore Oct 13 '16 at 14:06
  • \$\begingroup\$ @EaBangalore do you mean writing smth like "var scoreInputs = $('.js-question-score')" at the beginning of the script? \$\endgroup\$ – user3529607 Oct 13 '16 at 14:10
1
\$\begingroup\$

You can do it in only one event. Get sums of all value on each changement like this :

$(".js-question-score").change(function(){
  var res=0;
  $(".js-question-score").each(function()
  {
    res+=parseInt($(this).val());
  });
   $(".js-total-user-score").text(res);
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<form> 
  <input class="js-question-score" name="first" min="0" max="20"  value="0" type="number"/>
  <input class="js-question-score" name="second" min="0" max="20"  value="0" type="number"/>
  <input class="js-question-score" name="third" min="0" max="20"  value="0" type="number"/>
</form>

<div>
  User total score:
  <span class="js-total-user-score">
    0
  </span>
</div>

\$\endgroup\$
  • \$\begingroup\$ An improvement would be to avoid searching $(".js-question-score") inside the change event by wrapping the this as a jQuery object like so $(this). This way we would benefit from not traversing the DOM twice. \$\endgroup\$ – n0m4d Oct 13 '16 at 14:18
  • 1
    \$\begingroup\$ @n0m4d Nop because $(this) match only the selected element. \$\endgroup\$ – Alexis Oct 13 '16 at 14:20
  • 1
    \$\begingroup\$ You are definitely right! I completely overlooked that. Thanks for pointing that out =) \$\endgroup\$ – n0m4d Oct 13 '16 at 14:23
1
\$\begingroup\$

At first glance, your code looks alright. You could try to make it more compact but that will not make it more performant. In any case, and for a better understanding of what is going on with your code, I suggest you create a jsfiddle and share the link. One thing I could propose is to encapsulate your code, this will make it more readable and easier to maintain, something along the lines of this:

var scoreSystem = {

    init: function () {
        this.totalScore = 0;
        $('.js-question-score').change(function () {
            scoreSystem.totalScore();
            scoreSystem.updateScore();
        });
    },

    totalScore: function () {
        $('.js-question-score').each(function() {
            scoreSystem.totalScore += parseInt($(this).val());
        });
    },

    updateScore: function () {
        $('.js-total-user-score').text(scoreSystem.totalScore);
    }
};

$(function() {
    scoreSystem.init();
});

In terms of the delay you mention, can you elaborate? - Again, jsfiddle would be perfect for reviewing the problem.

\$\endgroup\$
  • \$\begingroup\$ Thx for encapsulation advice! I will try it. As for jsfiddle, i used code Snippet in my op post, isnt thats the same? I mean when i click "run code snippet" i see environment very similar ro jsfiddle. As for delay: when i change input value with clicking on arrows quickly, it takes about 1 sec to change result field. Mb thats the problem with my chromium and you dont see it.. \$\endgroup\$ – user3529607 Oct 13 '16 at 14:30
  • \$\begingroup\$ JSFiddle is an online service where we can see your entire code and test it like a real app. In any case, I am glad I was able to help! \$\endgroup\$ – Aichholzer Oct 13 '16 at 14:35
  • \$\begingroup\$ scoreSystem in closure is very very bad practice. why you use this.totalScore but scoreSystem.totalScore at the same time? \$\endgroup\$ – Maxx Oct 13 '16 at 15:27
  • \$\begingroup\$ @Maxx - How is it very very bad practice? - this.totalScore (yes, it can also be scoreSystem.totalScore right away) in the first place is due to the scope, since the other calls are inside functions, the scope of this does not exist anymore, hence the object call. \$\endgroup\$ – Aichholzer Oct 13 '16 at 22:14
1
\$\begingroup\$

If you want to hide your functions from the rest of the page, you should enclose them in an anonymous function. It works like this:

(function() {
  // your functions and code here
})();

Applied to your code, it is:

(function() {
    $('.js-question-score').change(function () {
        var totalScore = getTotalUserScore();
        updateTotalUserScore(totalScore);
    });

    function updateTotalUserScore(totalScore) {
        $('.js-total-user-score').text(totalScore)
    }

    function getTotalUserScore() {
        var totalScore = 0;
        $('.js-question-score').each(function() {
            totalScore += parseInt($(this).val());
        });
        return totalScore;
    }
})();

This has the effect that the functions updateTotalUserScore and getTotalUserScore are not visible to any other code on the page.

This style is easier to understand than the one suggested in another answer, where you have this structure:

var myFunctions = {
    getTotalUserScore: function() { ... },
    change: function() { myFunctions.getTotalUserScore(...); }
};

Oh, and by the way, never use parseInt(num), always use parseInt(num, 10). Because in the first form, the number 08 (which may appear in dates) leads to an error in older browsers.

\$\endgroup\$
0
\$\begingroup\$

Instead of this

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

Obtain a local copy of this for your server, This is nit picky, but better IMHO.

/Scripts/jquery-1.8.3.min.js
\$\endgroup\$
  • 1
    \$\begingroup\$ Unless you have your application fronted by a CDN, I would guess end user is going to typically get better download performance from Google's CDN. \$\endgroup\$ – Mike Brant Oct 14 '16 at 2:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy