0
\$\begingroup\$

I am always surprised by JavaScript and how simple and elegant JS code can look. I have a piece of code below that simulates a finance calculator. Please share your thoughts, critiques/feedback on how the code can be improved.

jsFiddle

The calculation is just a simulation, so please ignore. Also please ignore the way I display currency ($$$).

JavaScript:

$(function () {
 // we need to cache selectors to avoid re-reading the DOM
 var amount = $("#amount"),
     duration = $("#duration"),
     interest = $("#interest"),
     income = $("#incomeNeeded"),
     monthlyRate = $("#monthlyRate");

 var updateIncome = function() {
     var amountVal =amount.val().slice(0, -4), 
         durationVal = duration.val().slice(0, -6), 
         interestVal = interest.val().slice(0, 2);
     var resultIncome = amountVal +'-'+ durationVal +'-'+ interestVal;
     return resultIncome;
 };

 var updateRate = function() {
     var amountVal =amount.val().slice(0, -4), 
         durationVal = duration.val().slice(0, -6), 
         interestVal = interest.val().slice(0, 2);
     var resultRate = amountVal +'-'+ durationVal +'-'+ interestVal;
     return resultRate;
 };

 $("#slider-range-min").slider({
     range: "min",
     value: 10000,
     min: 1,
     max: 20000,
     slide: function (event, ui) {
         $("#amount").val(ui.value + " $$$");

         // seems like we can use either text() or html()
         income.html(updateIncome());
         monthlyRate.html(updateRate());
     }
 });
 $("#amount").val($("#slider-range-min").slider("value") + " $$$");

 $("#duration-range-min").slider({
     range: "min",
     value: 24,
     min: 1,
     max: 70,
     slide: function (event, ui) {
         $("#duration").val(ui.value + " months");
         income.html(updateIncome());
         monthlyRate.html(updateRate());
     }
 });
 $("#duration").val($("#duration-range-min").slider("value") + " months");

 $("#interest-range-min").slider({
     range: "min",
     value: 12,
     min: 4,
     max: 25,
     slide: function (event, ui) {
         $("#interest").val(ui.value + " %");
         income.html(updateIncome());
         monthlyRate.html(updateRate());
     }
 });
 $("#interest").val($("#interest-range-min").slider("value") + " %");
 });

HTML:

<div style="width: 500px;">
<p>
    <label for="amount">Max amount:</label>
    <input type="text" id="amount" class="input1">
</p>
<div id="slider-range-min"></div>
<p>
    <label for="duration">duration:</label>
    <input type="text" id="duration" readonly class="input1">
</p>
<div id="duration-range-min"></div>
<p>
    <label for="interest">Interest:</label>
    <input type="text" id="interest" readonly class="input1">
</p>
<div id="interest-range-min"></div>
<div style="text-align: center; float: left; width: 49%;">
    <p>Income needed:</p><span id="incomeNeeded" style="">x</span>

</div>
<div style="text-align: center; float: left; width: 49%;">
    <p>Monthly rate:</p><span id="monthlyRate" style="">x</span>

</div>

\$\endgroup\$
1
\$\begingroup\$

You have cached the selectors beforehand,

// we need to cache selectors to avoid re-reading the DOM
 var amount = $("#amount"),
     duration = $("#duration"),
     interest = $("#interest"),
     income = $("#incomeNeeded"),
     monthlyRate = $("#monthlyRate");

So why not use them here,

$("#amount").val($("#slider-range-min").slider("value") + " $$$");

and

slide: function (event, ui) {
     $("#amount").val(ui.value + " $$$");

to

amount.val($("#slider-range-min").slider("value") + " $$$");

and

slide: function (event, ui) {
     amount.val(ui.value + " $$$");

Repeat similarly for #duraton and #interest.


Also, instead of using .slice(x,y) where x and y are static values, why not use:

.slice(0,amount.val().indexOf(' '))

or use split() - Fiddle

JSPerf test- split vs slice vs substring

\$\endgroup\$
  • \$\begingroup\$ Did you mean .slice(0,amount.val().indexOf(' ')) \$\endgroup\$ – bg17aw May 27 '15 at 9:34
  • \$\begingroup\$ Ahh. Yes. Edited.! \$\endgroup\$ – Shaunak D May 27 '15 at 9:35
  • \$\begingroup\$ nice answer, I'll wait a few days to see if anything else comes up and mark it as the solution, thanks \$\endgroup\$ – bg17aw May 27 '15 at 9:50
1
\$\begingroup\$

There are a couple of things you could refactor out and improve.

As mentioned in other answers you cache the selectors so you should use them in the code, but also prefix variable storing jquery objects with a $ so it's clear that they hold a reference to jquery and not just a value:

var $amount = $("#amount")

Also, you functions updateRate and updateIncome do the some calculations so you are duplicating code, so it could just be one function, you should use a named function rather then anonymous to make debugging at a later date easier: (you also don't need to set a variable to return)

 function update () {
     var amountVal = $amount.val().slice(0, -4), 
         durationVal = $duration.val().slice(0, -6), 
         interestVal = $interest.val().slice(0, 2);
     return amountVal +'-'+ durationVal +'-'+ interestVal;
 };

UPDATE: in your original function you create a variable to return:

var resultIncome = amountVal +'-'+ durationVal +'-'+ interestVal;
return resultIncome;

Once you function has returned resultsIncome is no longer accessable, and when the function is called again the value held by resultsIncome is updated.

Because of that there is no need to set amountVal +'-'+ durationVal +'-'+ interestVal to a variable, you can just return amountVal +'-'+ durationVal +'-'+ interestVal, the function will return the same value either way, but you avoid an unnecessary variable and will slightly improve the performance.

IF performance becomes an issue then it would make sense to factor out the jquery for native js:

var amount = document.getElementById("amount"),
     duration = document.getElementById("duration"),
     interest = document.getElementById("interest"),
     income = document.getElementById("incomeNeeded"),
     monthlyRate = document.getElementById("monthlyRate");


     function update () {
         var amountVal = amount.value.slice(0, -4), 
             durationVal = duration.value.slice(0, -6), 
             interestVal = interest.value.slice(0, 2);
         return amountVal +'-'+ durationVal +'-'+ interestVal;
     };

Then just use jquery for the sliding functionality, as doing that in js would mean more code and handling of browser inconsistencies.

jquery object can be made out of stored objects by passing them to $, such as

$("#slider-range-min") becomes $(document.getElementById('slider-range-min'));

That might seem like more code, but the performance for selecting id's like this is about 30/40% better

\$\endgroup\$
  • \$\begingroup\$ you also don't need to set a variable to return - can you explain this a bit more? \$\endgroup\$ – bg17aw May 28 '15 at 14:54
  • \$\begingroup\$ have updated to explain the return variable a bit more \$\endgroup\$ – atmd May 28 '15 at 15:06
1
\$\begingroup\$

The first thing you might want to do it put all of your code into an IIFE to create a private scope for yourself that way you aren't polluting the global namespace. Since you are using jQuery, you can pass that into the IIFE to make sure that $ always refers to jQuery.

(function( $ ){
  //your code here
})( jQuery );

As pointed out above you should cache all of your selectors including your sliders:

var $amount = $("#amount"),
    $duration = $("#duration"),
    $interest = $("#interest"),
    $income = $("#incomeNeeded"),
    $monthlyRate = $("#monthlyRate"),
    $sliderMin = $("#slider-range-min"),
    $durationMin = $("#duration-range-min"),
    $interestMin = $("#interest-range-min");

Also, all of your code is in the document.ready area. Does it all need to be in there? You can probably define a lot of what you want to do before the load event and then kick it off when the load event fires.

$(function(){
  initialize();  // or whatever 
});

In this function you can cache all your selectors, create the initial state, etc.

A lot of your code is repeated or very very similar. You can DRY them out a bit. For example, both updateAmount and updateRate are the exact same function. Also they return a value just so another function can update the screen. You can combine all of that into one function:

var updateAmount = function() {
  var amountVal = $amount.val().split(' ')[0], //as shown by other answer
      durationVal = $duration.val().split(' ')[0],
      interestVal = $interest.val().split(' ')[0],
      display;
  display = amountVal +'-'+ durationVal +'-'+ interestVal;;
  $income.html(display);
  $monthlyRate.html(display);
};

You can use this function instead of the two functions above.

Another repeated part is this line:

$("#amount").val($("#slider-range-min").slider("value") + " $$$");
$("#duration").val($("#duration-range-min").slider("value") + " months");
$("#interest").val($("#interest-range-min").slider("value") + " %");

We already cached those values so they could be re-written as:

$amount.val($sliderMin.slider("value") + " $$$");
$duration.val($durationMin.slider("value") + " months");
$interest.val($interestMin.slider("value") + " %");

Done this way you can see that this can be DRY-ed into a single function:

var setSliderValue = function ($el, $slider, txt) {
  $el.val( $slider.slider("value") + txt );
};

setSliderValue( $duration, $durationMin, " months" );
setSliderValue( $amount, $sliderMin, " $$$" );
setSliderValue( $interest, $interestMin, " %" );

Also, repeated is the actual slide function on each slider implementation. So you could create a single function and change to something like this:

var slideHandler = function( $el, $slider, txt ) {
  updateAmount();
  setSliderValue( $el, $slider, txt );
};

$sliderMin.slider({
  range: "min",
  value: 10000,
  min: 1,
  max: 20000,
  slide: function () {
    slideHandler( $amount, $sliderMin, " $$$" );
  }
});

Create another function to initialize each slider as well:

var initSlider = function( $el, $fld, txt, val, min, max ) {
  $el.slider({
    range: "min",
    value: val,
    min: min,
    max: max,
    slide: function () {
      slideHandler( $fld, $el, txt );
    }
  });
};

After all of those changes, here is the final markup.

(function( $ ) {

  var $amount, $duration, $interest, $income, $monthlyRate,
      $sliderMin, $durationMin, $interestMin; 

  var updateAmount = function() {
    var amountVal = $amount.val().slice(0, -4),
        durationVal = $duration.val().slice(0, -6),
        interestVal = $interest.val().slice(0, 2),
        display;
    display = amountVal +'-'+ durationVal +'-'+ interestVal;
    $income.html(display);
    $monthlyRate.html(display);
  };

  var slideHandler = function($el, $slider, txt) {
    updateAmount();
    setSliderValue($el, $slider, txt);
  };

  var setSliderValue = function ($el, $slider, txt) {
    $el.val($slider.slider("value") + txt);
  };

  var initSlider = function( $el, $fld, txt, val, min, max ) {
    $el.slider({
      range: "min",
      value: val,
      min: min,
      max: max,
      slide: function () {
        slideHandler( $fld, $el, txt );
      }
    });
  };

  var initialize = function() {

    $amount = $("#amount");
    $duration = $("#duration");
    $interest = $("#interest");
    $income = $("#incomeNeeded");
    $monthlyRate = $("#monthlyRate");
    $sliderMin = $("#slider-range-min");
    $durationMin = $("#duration-range-min");
    $interestMin = $("#interest-range-min");

    initSlider( $sliderMin, $amount, " $$$", 10000, 1, 20000 );
    initSlider( $durationMin, $duration, " months", 24, 1, 70 );
    initSlider( $interestMin, $interest, " %", 12, 4, 25 );

    setSliderValue( $duration, $durationMin, " months" );
    setSliderValue( $amount, $sliderMin, " $$$" );
    setSliderValue( $interest, $interestMin, " %" );

  };

  $(function () {
    initialize();
  });
})( jQuery );

Let me know if you have questions or comments. Here is the updated fiddle

Hope that helps!

\$\endgroup\$
  • \$\begingroup\$ Very helpful, but why do you say you cannot test the code, you could fork/update my jsfiddle. Also you say "Since you are using jQuery, you can pass that into the IIFE to make sure that $ always refers to jQuery." but the final version of your code is not showing that. Probably as many others just starting to understand the power of IIFEs, I started using them without any parameters (a bit confusing passing parameters to IIFEs). Definitely I would love to learn what is the best practice (I would say possible, as usually there are multiple approaches each with their own merits). \$\endgroup\$ – bg17aw May 29 '15 at 8:48
  • \$\begingroup\$ "Also, all of your code is in the document.ready area. Does it all need to be in there? You can probably define a lot of what you want to do before the load event and then kick it off when the load event fires." I had problems in the past trying to access elements that where not on the page yet, especially when placing the JS code at the top of the page (header). Now I place JS before the closing body tag and check for document.ready. Placing everything inside document.ready would allow me to place the code in the header section, for example, right? \$\endgroup\$ – bg17aw May 29 '15 at 9:18
  • \$\begingroup\$ 3rd question, sorry: Someone recommended earlier you should use a named function rather then anonymous to make debugging at a later date easier. You didn't picked up on this, what is your take on this? Thanks \$\endgroup\$ – bg17aw May 29 '15 at 9:25
  • \$\begingroup\$ 1 There is an IIFE in this code. It is the first and last line. 2) The document.ready ( $(function() { .. }); at the bottom) only has the one function call. But since it is inside the IIFE it is part of the same scope and can call all of the other functions. But those functions/vars are not available to functions outside this scope. 3) Yes, I agree with this. In the above code there are no anonymous functions. Here is a link to a video that talks about scope and closures (an IIFE is a type of closure). \$\endgroup\$ – Gary Storey May 29 '15 at 14:25
  • \$\begingroup\$ Oh, you should still leave all the scripts at the bottom of your HTML right before the closing body tag. When browsers encounter a JS script in the head they immediately parse the JS before continuing to render the page. This will slow your site down. So for site performance reasons leave it at the bottom. \$\endgroup\$ – Gary Storey May 29 '15 at 14:31

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.