6
\$\begingroup\$

I've written it for an assessment. It works but my assessment also requires a peer to review it in terms of how it could be improved, how it rates as a piece of JavaScript, etc, any comments would be appreciated (I don't know anyone else to ask).

As a bit of background this is JavaScript written to be embedded in a HTML doc. It calculates the balance of two accounts when they each increment daily by a certain value. It reports when they will be equal, when the lesser balance exceeds the greater, or if the accounts will always be unequal. All values can be entered by a user.

I look forward to your comments.

/process function is called by user pressing a Process button/

function process()
{
var account1 = document.getElementById("account_1").value;   
var account2 = document.getElementById("account_2").value;   
var increm1 = document.getElementById("increm_1").value;    
var increm2 = document.getElementById("increm_2").value;

blankOrSame(account1,account2,increm1,increm2); 

account1 = Number(account1);
account2 = Number(account2);
increm1 = Number(increm1);
increm2 = Number(increm2);

alwaysDiff(account1,account2,increm1,increm2);

sameGreater(account1,account2,increm1,increm2);
}

/*field_vldn function is performed onchange when user updates fields*/

function field_vldn(name) //onchange validation for account & increment fields.
{
var num = document.getElementById(name).value;
if ( isNaN(num))
    {
    alert("Please enter a number.");
    return document.getElementById(name).value = null;  
    }
}

/below functions are called by process function/

function blankOrSame(account1,account2,increm1,increm2)//validation for blank fields or if accounts will always be equal.
{
    if (account1 == "" || account2 == "" || increm1 == "" || increm2 == "") 
        {
        alert("Please complete all fields then press the 'Process' button.");   
        } 
    else if (account1 == account2 && increm1 == increm2) 
        {
        alert("Based on the values entered the both Accounts will always have an equal balance.");
        }
}


function alwaysDiff(account1,account2,increm1,increm2)//determined if accounts will always be inequal. Variables must first be converted to numbers.
{
    if ((account1 <= account2 && increm1 < increm2) || (account1 < account2 && increm1 == increm2))
        {
        alert("The accounts will never be equal. Account 1 will always contain less money than Account 2.");
        }
    else if ((account1 >= account2 && increm1 > increm2) || (account1 > account2 && increm1 == increm2))
        {
        alert("The accounts will never be equal. Account 2 will always contain less money than Account 1.");
        }
}

function sameGreater(account1,account2,increm1,increm2)//determines when accounts will be same or lesser exceeds greater.
{
    if (account1 < account2 && increm1 > increm2)//statement for when account 1 is initally lesser.
        {
        var n = 0;
        while (account1 < account2)
            {
            account1 = account1 + increm1;
            account2 = account2 + increm2;
            n = ++n
            }
        if (account1 == account2)//accounts eventually become equal, will also report when account1 exceeds account2.
            {
            alert("After " + n + " day/s Account 1 and Account 2 will be the same balance ($" + account1 + ").\nIn "
            + (n+1) + " day/s Account 1 ($" + (account1+increm1) + ") will be greater than Account 2 ($" + (account2+increm2)
            + ").");
            }
        else if (account1 > account2)//account1 exceeds account2 without ever becoming equal.
            {
            alert("After " + n + " day/s Account 1($" + account1 + ") will be greater than Account 2 ($" + account2 + ").");
            }
        }
    else if (account1 > account2 && increm1 < increm2)//statement for when account 2 is initally lesser.
        {   
        var n = 0;
        while (account1 > account2)
            {
            account1 = account1 + increm1;
            account2 = account2 + increm2;
            n = ++n
            }
        if (account1 == account2)//accounts eventually become equal, will also report when account2 exceeds account1.
            {
            alert("After " + n + " day/s Account 1 and Account 2 will be the same balance ($" + account1 + ").\nIn "
            + (n+1) + " day/s Account 2 ($" + (account2+increm2) + ") will be greater than Account 1 ($" + (account1+increm1)
            + ").");
            }
        else if (account1 < account2)//account2 exceeds account1 without ever becoming equal.
            {
            alert("After " + n + " day/s Account 2 ($" + account2 + ") will be greater than Account 1 ($" + account1 + ").");
            }

        }
}
\$\endgroup\$
4
  • \$\begingroup\$ document.getElementById may not work on all of the js enabled browsers. See e.g. this Q/A: stackoverflow.com/questions/4069982/… \$\endgroup\$
    – Helium
    Commented Apr 10, 2014 at 10:28
  • \$\begingroup\$ if (account1 == "" || account2 == "" || increm1 == "" || increm2 == "") is a bad way to check the fields. What if the user inputs something other than a number? e.g., an space char? \$\endgroup\$
    – Helium
    Commented Apr 10, 2014 at 10:30
  • \$\begingroup\$ @Mohsen You are wrong, and that link does not state that. Please check developer.mozilla.org/en-US/docs/Web/API/… \$\endgroup\$
    – konijn
    Commented Apr 10, 2014 at 15:35
  • \$\begingroup\$ There are incompatibilities in some older browsers, such as IE 6 and 7 and it's safer to use jQuery to select an elelment: remysharp.com/2007/02/10/ie-7-breaks-getelementbyid \$\endgroup\$
    – Helium
    Commented Apr 11, 2014 at 4:23

1 Answer 1

4
\$\begingroup\$
  1. In JavaScript, it's generally a good idea to use the brace-on-same-line style, i.e.

    if( ... ) {
      ...
    } else {
      ...
    }
    

    because of javascript's policy of automatic semicolon insertion. While it's usually not a problem, it may bite you one day if you use the brace-on-new-line style.

  2. field_vldn is a strange name - your other functions are named more descriptively, and use the conventional camelCase style - why doesn't field_vldn do the same?

  3. Your validation is being done in two places (field_vldn and in blankOrSame). Combine that logic.

  4. The logic for comparing the accounts is too complicated. Or, each part of it is simple, but there's too much repetition, and using the while loops (two of them even, instead of extracting a function) is very brute-force, when a more generic solution is possible.

For that last point, here's the deal: this is a simple math problem. An account's balance can be expressed as:

$$ f(x) = ax + b $$

where \$x\$ is the number of days, \$a\$ is the daily increment, and \$b\$ is the initial balance.

To figure out when - if ever - the two accounts, \$f{_1}\$ and \$f{_2}\$, will be equal, you simply isolate \$x\$ in \$f{_1}(x) = f{_2}(x)\$:

$$ x = \frac{b{_2} - b{_1}}{a{_1}-a{_2}} $$

If \$x\$ is zero, the accounts are already equal at day 0.
If \$x\$ is negative, the accounts won't ever be equal (unless you go back in time).
If \$x\$ is positive and an integer, the two accounts will end up being exactly equal after \$x\$ days.
If \$x\$ is positive but fractional, you simply round up to the next integer, and that's the number of days it'll take for one account to "overtake" the other.
If \$x\$ is ±Infinity (which is how JS expresses \$\frac{n}{0}\$, and which you can check with Number.isFinite()) the two balances will progress in parallel, and will never be equal or intersect.
If \$x\$ is NaN, the two accounts are identical (\$x = \frac{0}{0}\$) and will always be equal.

Note that while you can simply use x === Infinity instead of !Number.isFinite(), you can't use x === NaN or x == NaN. In JavaScript NaN isn't equal to anything - it's not even equal to NaN (don't ask why).

You can also calculate the numerator and denominator of the fraction separately, and do some checks there instead of doing the division. Once you have the mathematical approach, you can make some practical optimizations.

\$\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.