6
\$\begingroup\$

I created a program that determines how many years you have left before retiring, and in a year when you can retire.

This is my code below:

$(function(){
	const currentYear = (new Date).getFullYear();
	var convert = function(a){
	    return parseInt(a.val());
	}
	var result = function(a, b){
	    $('.currentYear').text(currentYear);
	    if (b>a){
		$('.newAge').text(b - a);
		$('.retireYear').text(currentYear + (b - a));
		message1.dialog("open");
		return message1
	    } else {
		$('.newAge').text(a - b);
		$('.retireYear').text(currentYear - (a - b));
		message2.dialog("open");
		return message2
	    }
	}
	const message1 = $("#message1").dialog({autoOpen: false});
	const message2 = $("#message2").dialog({autoOpen: false});
        const form = $("#form").dialog({
            autoOpen: false,
            modal: true,
            buttons:[{
            	text: "Сount",
            	icon: "ui-icon-contact",
            	click: function(){
            	    $( this ).dialog( "close" );
                    var str1 = convert($('#currentAge'));
                    var str2 = convert($('#retireAge'));
                    result(str1, str2);
            	}
            }]
        });
        $("#btn").click(function(){
    	    form.dialog("open");
    	});
});
<link href="https://code.jquery.com/ui/1.12.1/themes/base/jquery-ui.css" rel="stylesheet"/>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<script src="//code.jquery.com/ui/1.12.1/jquery-ui.js"></script>
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-alpha.6/css/bootstrap.min.css" integrity="sha384-rwoIResjU2yc3z8GV/NPeZWAv56rSmLldC3R/AZzGRnGxQQKnKkoFVhFQhNUwEyJ" crossorigin="anonymous">

<body>
<div class="container text-center m-5">
	<button type="button" class="btn btn-primary" id="btn">Retirement Calculator</button>
</div>

<form id="form">
    <div class="form-group">
        <label>What is your current age?</label>
        <input type="number" class="form-control" id="currentAge" oninput="validity.valid||(value='');" min="0" style="width: 90px">  
    </div>
    <div class="form-group">
        <label>At what age would you like to retire?</label>
        <input type="number" class="form-control" id="retireAge" oninput="validity.valid||(value='');" min="0" style="width: 90px">  
    </div>
</form>

<div id="message1">
	<p>You have <span class="newAge"></span> years left until you can retire.</p>
	<p>It's <span class="currentYear"></span>, so you can retire in <span class="retireYear"></span>.</p>
</div>
<div id="message2">
	<p>You could retire <span class="newAge"></span> years ago.</p>
	<p>It's <span class="currentYear"></span>, so you could retire in <span class="retireYear"></span>.</p>
</div>
</body>

I will appreciate any advice about code.

\$\endgroup\$
1
  • \$\begingroup\$ I don't really see any financial calculations so I don't know if I'd call it a "retirement" calculator. \$\endgroup\$
    – Raystafarian
    Commented Feb 28, 2018 at 21:15

2 Answers 2

Reset to default
5
\$\begingroup\$

  • Since you're using some ES6+ features, I'd definitely recommend looking into others, such as let, arrow functions, and so on. This website is a good start, and you can find more information by searching "javascript es6".

  • Adding onto the last point, whenever you don't change a variable, prefer using const for consistency's sake. If you do need to change a variable, use let. Note that you can change properties on objects declared with const, but you still can't reassign the variable. You can look up more details on the website I linked.

  • The result function can be improved.

    First off, it doesn't look like the return value is being used at all, so there's no need to return anything from it.

    Secondly, if an element is referenced multiple times from jQuery, it's usually a good idea to cache it in a variable, so jQuery doesn't have to unnecessarily query the DOM multiple times. In small cases like this the performance difference doesn't matter much, but it's still a decent habit to pick up.

Here's what you'd end up with for the result function, as an example:

const result = (a, b) => {
  const $newAge = $(".newAge")
  const $retireYear = $(".retireYear")

  $(".currentYear").text(currentYear)
  if (b > a) {
    $newAge.text(b - a)
    $retireYear.text(currentYear + (b - a))
    message1.dialog("open")
  } else {
    $newAge.text(a - b)
    $retireYear.text(currentYear - (a - b))
    message2.dialog("open")
  }
}
\$\endgroup\$
2
\$\begingroup\$

Parsing integers

The function convert() calls parseInt() with no second parameter (i.e. radix). The MDN documentation states one should "Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior"1. So that call should have 10 added as the second parameter. And that function doesn't get re-assigned, so const can be used instead of var.

const convert = function(a){
    return parseInt(a.val(), 10);
}

Caching DOM references

I agree with kingdao's answer: DOM references should be cached, though they should be cached once per page load instead of each time the function runs. So in the example code, $newAge and $retireYear should be declared outside the result function, and inside the DOM-ready callback (i.e. $(function(){ ...})).

Also, the names might be a little misleading. For example, the jQuery function (i.e. $(selector)) returns a jQuery object referencing DOM elements. Thus an appropriate name might be newAgeElements instead of $newAge (though you can use the $ if you want). Also, cache the currentYear elements in a constant too.

$(function() {  //DOM ready callback
    const newAgeElements = $(".newAge");
    const retireYearElements = $(".retireYear");
    const currentYearElements = $(".currentYear");
    //other variables/constants declaration: like currentYear, convert()

    const result = (a, b) => {

      currentYearElements.text(currentYear)
      if (b > a) {
        newAgeElements.text(b - a)
        retireYearElements.text(currentYear + (b - a))
        message1.dialog("open")
      } else {
        newAgeElements.text(a - b)
        retireYearElements.text(currentYear - (a - b))
        message2.dialog("open")
      }
    }
});

For more information about why caching DOM elements is important, read this article. I know it is a few years old and bashes jQuery but it is still relevant and useful.

Templating

It might be simpler to use a template to for the text, either in the markup (i.e. HTML) or stored in a string literal (perhaps a template literal would be a good application here, though note the Browser compatibility!). See the answers to this SO question for more ideas about possible popular libraries you could use or other plain Javascript features that could be used.

Then instead of updating the text of multiple HTML elements, the template could be used to add the values and the HTML could be simplified to only having one panel for the result instead of two, which would also allow you to simplify the conditional Javascript code.

1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Parameters

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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