Commute cost calculator

Question

I wrote a simple javascript program that takes a user's input and calculates the cost of their commute. I feel like I'm repeating a lot of stuff though.

How could I refactor this to make it cleaner and more concise?

function calcCost(costOfGas, vehicleMPG, numMiles) {
  var dailyCost = (costOfGas / vehicleMPG) * (numMiles * 2);
  var weeklyCost = dailyCost * 5;
  var monthlyCost = weeklyCost * 4;

  return {
    dailyCost: dailyCost,
    weeklyCost: weeklyCost,
    monthlyCost: monthlyCost
  };
}

var form = document.forms[0];

form.onsubmit = function(e) {
  e.preventDefault();

  if (form.costOfGas.value == '' || 
    form.vehicleMPG.value == '' ||
    form.numMiles.value == '') 
  {
    return;
  } 
  else 
  {
    var userCostOfGas = +form.costOfGas.value;
    var userMPG = +form.vehicleMPG.value;
    var userNumMiles = +form.numMiles.value;

    form.costOfGas.value = '';
    form.vehicleMPG.value = '';
    form.numMiles.value = '';

    var costs = calcCost(userCostOfGas, userMPG, userNumMiles);

    var inputReturned = document.getElementById("inputReturned");
    inputReturned.innerHTML = "For a car that gets " + userMPG + 
    " MPG, driving " + userNumMiles + " miles to work each day at $" + 
    userCostOfGas.toFixed(2) + "/gal:";

    calculatedDailyCost.innerHTML = "You're spending $" + costs.dailyCost.toFixed(2) + " on gas every day.";
    calculatedWeeklyCost.innerHTML = "You're spending $" + costs.weeklyCost.toFixed(2) + " on gas every week.";
    calculatedMonthlyCost.innerHTML = "You're spending $" + costs.monthlyCost.toFixed(2) + " on gas each month.";
  }
};

Answer 1

You have inconsistent bracing style. For example, in your onsubmit function, you start the brace on the same line as the function signature:

form.onsubmit = function(e) { // <=======

But, in this if/else conditional, you are starting the brace on the next line:

else 
{ // <========

Choose one and stick with it: it makes your code a lot more clean. Personally, I like to use the first version.

---

Here, you are checking to make sure that the form is not blank:

if (form.costOfGas.value == '' || 
  form.vehicleMPG.value == '' ||
  form.numMiles.value == '') 
{

However, there is a much, much easier way to do this: using the required HTML attribute (scroll down for the "required" section)

By adding a blank required attribute to each of the input elements of your form, you can safely remove this chunk of code from your JavaScript; the user will no longer be able to submit the form unless they have entered data in each field.

---

Here, you are using the + operator to convert a string to a number:

var userCostOfGas = +form.costOfGas.value;

While this is a neat little JavaScript trick, it looks like it's more for if you were creating this code for a code golf: it is very unclear as to what this is doing.

To make this more clear, use the parseInt function:

var userCostOfGas = parseInt(form.costOfGas.value, 10);

---

It looks to me that, since you are attempting to turn the form input fields from a string into a number, that you are using input fields of the type text.

By doing it this way, you are forcing yourself to take that extra step of converting the input to a number type. It would a lot easier if you set the type attribute of your input fields to "number".

That way, when you go to get the value of the input fields, they will already be of the correct type.

---

Good job using toFixed on the output: it makes the formatting of numbers very clean.

---

You have a few magic numbers in your code, especially in the calcCost function:

var dailyCost = (costOfGas / vehicleMPG) * (numMiles * 2);
var weeklyCost = dailyCost * 5;
var monthlyCost = weeklyCost * 4;

Where did the 2 come from? What about the 5? And the 4?

To make your code more readable and understandable, you should declare constants at the top of your code, and then use the constants in place of these mysterious numbers.

Here, I've created a few for you:

var DAYS_IN_WEEK = 5;
var WEEKS_IN_MONTH = 4;

I wasn't able to figure out where the 2 came from.

Then, going back to the code snippet, you would use these constant variables like this:

var weeklyCost = dailyCost * DAYS_IN_WEEK;
var monthlyCost = weeklyCost * WEEKS_IN_MONTH;

Answer 2

1. You're repeating form.costOfGas, form.vehicleMPG, etc... when you can just define them earlier on

2. HTML5 has a required attribute you can use - in which case you don't need the extra check in your JS code, but if you can't modify your HTML, an easier way is to use ! - an empty string is falsey, so it works.

3. calculatedDailyCost, calculatedWeeklyCost,calculatedMonthlyCost aren't defined...

4. To set the values as empty, simply do costOfGas.value = vehicleMPG.value = numMiles.value = ''; - in my opinion, it's easier to see what's happening and shorter.

5. You can significantly reduce the amount of code by doing away with all the user... variable if you empty the input fields at the end!

Here's the code with these points taken into account (other than number 3):

function calcCost(costOfGas, vehicleMPG, numMiles) {
    var dailyCost = (costOfGas / vehicleMPG) * (numMiles * 2);
    var weeklyCost = dailyCost * 5;
    var monthlyCost = weeklyCost * 4;

    return {
        dailyCost: dailyCost,
        weeklyCost: weeklyCost,
        monthlyCost: monthlyCost
    };
}

var form = document.forms[0];

form.onsubmit = function (e) {
    e.preventDefault();
    var costOfGas = form.costOfGas.value;
    var vehicleMPG = form.vehicleMPG.value;
    var numMiles = form.numMiles.value;

    if (!costOfGas || !vehicleMPG || !numMiles) {
        return;
    } else {            
        var costs = calcCost(+costOfGas, +vehicleMPG, +numMiles);

        var inputReturned = document.getElementById("inputReturned");
        inputReturned.innerHTML = "For a car that gets " + vehicleMPG +
            " MPG, driving " + numMiles + " miles to work each day at $" + costOfGas.toFixed(2) + "/gal:";

        calculatedDailyCost.innerHTML = "You're spending $" + costs.dailyCost.toFixed(2) + " on gas every day.";
        calculatedWeeklyCost.innerHTML = "You're spending $" + costs.weeklyCost.toFixed(2) + " on gas every week.";
        calculatedMonthlyCost.innerHTML = "You're spending $" + costs.monthlyCost.toFixed(2) + " on gas each month.";

        costOfGas.value = vehicleMPG.value = numMiles.value = ''; //empty inputs *after* the results have been shown.
    }
};