What am I doing wrong?

[JeffreyBrianPaz]

https://pastebin.com/t2UQqUPc

Is my code but I can't get the future value or depreciation table forms to show. My professor briefly went over javascript so I've been teaching myself. Not sure how to make this work.

[Paul_Wilkins]

I've moved your code to a jsfiddle, so that it may be easily explored, over at https://jsfiddle.net/pmw57/4bmcfsjn/

The <div> and <form> at the end of the HTML code were around the wrong way, so I've switched those around and used the Tidy button to tidy up the code, making it easier to work with and understand.

After entering values for property value and property life, clicking Show Table is where the problem is experienced.

Looking at the browser console, we're told that it Cannot read property 'value' of undefined at the following line:

value = eval(document.depForm.pValue.value);

There is absolutely no reason to use eval anywhere in your code. You are using it to convert the string to a value, so we can use Number to so that in a much safer way instead. The variables are also being defined here, so I'll use var to declare them here too.

  var value = Number(document.depForm.pValue.value);
  var life = Number(document.depForm.pLife.value);

The standard way to get form values is to use a unique identifier on the form itself, and then use the elements collection to obtain information about the form.

Here the form has a name of depForm which is useless, so we'll make that an id attribute instead.

  <form name="depForm">

We can access values more properly from the form:

  var form = document.querySelector("#depForm");
  value = Number(form.elements.pValue.value);
  life = Number(form.elements.pLife.value);

The two fields that are being obtained are the property value, and the property life. Here's their HTML code:

    <br> Enter Property Value:
    <input type="text" name="value" id="value" value="">
    <br> Enter Property Life:
    <input type="text" name="life" id="life" value="">

The name of those desired form elements don't match the names in the form, which is the main cause of the problem.

Looking through the rest of your code I see that pValue and pLife are also used in other places, so renaming the HTML form values is the best course of action here.

    <br> Enter Property Value:
    <input type="text" name="pValue" id="value" value="">
    <br> Enter Property Life:
    <input type="text" name="pLife" id="life" value="">

And the code now works. Here's the working code with that change in place: https://jsfiddle.net/pmw57/4bmcfsjn/1/

[Paul_Wilkins]

Just having working code though isn't good enough. We can use JSLint to help us find weaknesses in our code and fix those up, to help improve it.

I've first run the code through jsbeautifier to tidy up the majority of formatting issues, so that JSLint only gives us more useful details about the structure of the code itself.

Quotes

You've used a mix of single and double quotes in the code. It's best to be consistent using only double quotes throughout your JavaScript code, so the following line gets improved to use double quotes as well:

    var table = document.getElementById("depTable");

For loops

The linter warns us about the for loops. They are an unnecessary detail that that should be removed. Here's one of them.

    for (var i = table.rows.length - 1; i > 0; i--) {
        table.deleteRow(i);
    }

I also see that there's a logical problem in that loop, for it always leaves the first row (row 0) in place. You don't want that to occur.
Using a while loop will completely fix that problem, and make the code easier to understand too:

    // for (var i = table.rows.length - 1; i > 0; i--) {
    while (table.rows.length > 0) {
        table.deleteRow(0);
    }

In other situations, using forEach to refer to each item in a collection is a better technique to use too.

use strict

To help protect your code, it's best to start functions with "use strict" which places your code in strict mode which helps to protect others, and help to warn you about more errors.

I will temporarily add "use strict" to all functions, but have a plan so that only one "use strict" is needed.
How we do that is to place your code inside of an IIFE (immediately invoked function expression) which also prevents your functions from being included in the global namespace.

We can now remove that "use strict" from all of the functions, and place it just below the iife wrapper function instead:

(function iife() {
    "use strict";
    // rest of code in here
}());

Unused functions

JSLint tells us that the functions aren't being used. It's not appropriate to use inline HTML events as that's mixing JavaScript code in with your HTML code. Instead of that, we move the JavaScript code down to the end of the body (just before the </body> tag) which also helps to improve page loading performance, and replace the HTML events with JavaScript events instead.

The big idea here is that instead of the HTML deciding what occurs, the JavaScript is used instead to control which functions get assigned to what elements.

I've told JSFiddle to place the script in the body instead of the head, and can now start to fix things.

ComputeFV from JavaScript instead of from HTML

Currently the ComputeFV function is called from an inline HTML event attribute, which needs to be removed and handled from JavaScript instead.

    <!-- <input type="button" value="ComputeFV" name="btnCompute" onclick="ComputeFV()" /> -->
    <input type="button" value="ComputeFV" name="btnCompute" />

To obtain a reference to that button, it's best to do if via a unique identifier on the form. So I'll remove name from the form element (which serves no purpose) and use the id there instead.

  <!-- <form name="fvForm"> -->
  <form id="fvForm">

We can now obtain a reference to that button from the JavaScript code, and attach the ComputeFV() function to it from there.

    var fvForm = document.querySelector("#fvForm");
    var fvButton = fvForm.elements.btnCompute;
    fvButton.addEventListener("click", ComputeFV);

The compute function shouldn't take elements from anywhere on the page. Instead, it should retrieve them from the elements collection of the form.

    function ComputeFV(evt) {
        var button = evt.target;
        var form = button.form;
        // myPV = parseFloat(document.getElementById("PV").value);
        // myRate = parseFloat(document.getElementById("Rate").value);
        myPV = parseFloat(form.elements.PV.value);
        myRate = parseFloat(form.elements.Rate.value);

The following code attempts to get the checked radio value:

        if (document.getElementById("Year10").checked) {
            myYear = 10;
        } else if (document.getElementById("Year15").checked) {
            myYear = 15;
        } else {
            myYear = 30;
        }

which can be done much more effectively by using the RadioNodeList value instead.

        var myYear = form.elements.Year.value;

I can even tell JavaScript to turn that string value from the form into a number:

        var myYear = Number(form.elements.Year.value);

Using form.elements.FV.value to set the value, the whole updated function now looks like this:

    function ComputeFV(evt) {
        var button = evt.target;
        var form = button.form;
        var myPV = parseFloat(form.elements.PV.value);
        var myRate = parseFloat(form.elements.Rate.value);
        var myYear = Number(form.elements.Year.value);
        var fv = myPV * Math.pow(1 + myRate, myYear);
        form.elements.FV.value = fv.toString();
    }

ShowTable from JavaScript instead of HTML

We also need to have ShowTable() handled from JavaScript instead of from an inline HTML event attribute:

    <!-- <input type="button" value="Show Table" onclick="showTable()" /> -->
    <input type="button" name="showTable" value="Show Table" />

    var depForm = document.querySelector("#depForm");
    var showTableBtn = depForm.elements.showTable;
    showTableBtn.addEventListener("click", ShowTable);

The showTable function can now use the evt.target to find the button, and from there gain a reference to the form. This way, a minimum of assumptions are made about the form.

    function showTable(evt) {
        var button = evt.target;
        var form = button.form;

The "use strict" is now being helpful and gives warnings that value, live, and depreciation aren't defined, so we need to use the var keyword with them:

        var value = Number(form.elements.pValue.value);
        var life = Number(form.elements.pLife.value);
        var depreciation = value / life;

count also needs to be defined using var, in the loop, to get things working:

        for (var count = 1; count <= life; count++) {

Now that the code works once more, we go back to JSLint to get more things that need to be fixed with the code:

Unexpected for

I knew that the for loop wasn't long for this world.

        for (var count = 1; count <= life; count++) {

Is count being used anywhere in the code? If it's not, then we can use another simpler method.

           // cell0.innerHTML = count + 1;
           cell0.innerHTML = rowCount + 1;

count is no longer being used anywhere in the code.

        while (life > 0) {
            // ...
            life -= 1;
        }

Fancier solutions exist, but those can be experimented with after the linting is complete.

Undeclared document

The document variable is always assumed to exist when using a web browser, so in the Assume... section of the linting page, we can tick the [✓] a browser box.

it's not good for assumptions to occur, but given that you're only planning on that script to be run in a web browser, it's okay at this stage to assume.

Don't declare variables in a loop

All of those variables declared in the while loop, shouldn't be there. Instead of moving the variable declarations up out of the loop, it seems more appropriate in this case to put them all inside of a function instead.

I want to separate working out the values, from the act of creating a row with those values.
When creating that row, it needs three values of value, depreciation, and totalDepreciation:

        function addRow(table, value, depreciation, totalDepreciation) {
        }

Having no function parameters is best, one or two parameters for a function is good, three of them isn't so good and four or more is bad.

We can move those value/depreciation/totalDepreciation values to an object or an array instead. Because we care about the ordering of them, and we don't want the addRow function to worry about that, using an array makes sense.

In fact, we can call it addNumberedRow so that it automatically adds the first numbered column, which helps to simplify things further:

        function addNumberedRow(table, values) {
        }
        ...
            addNumberedRow(table, [
                value.toFixed(2),
                depreciation.toFixed(2),
                totalDepreciation.toFixed(2)
            ]);
            value -= depreciation;

We can now move lines of code into that function:

        function addNumberedRow(table, values) {
            var rowCount = table.rows.length;
            var row = table.insertRow(rowCount);
            var cell0 = row.insertCell(0);
            cell0.innerHTML = rowCount + 1;
            var cell1 = row.insertCell(1);
            cell1.innerHTML = "$" + values[0];
            var cell2 = row.insertCell(2);
            cell2.innerHTML = "$" + values[1];
            var cell3 = row.insertCell(3);
            cell3.innerHTML = "$" + values[2];
        }

We can now simplify that function using forEach instead, which gives us:

        function addNumberedRow(table, values) {
            var rowCount = table.rows.length;
            var row = table.insertRow(rowCount);
            [rowCount, ...values].forEach(function addCell(value, index) {
              var cell = row.insertCell(index);
              cell.innerHTML = value;
            });
        }

and the rest of the code in the while loop becomes relatively simple too:

        while (life > 0) {
            totalDepreciation += depreciation;
            addNumberedRow(table, [
                value.toFixed(2),
                depreciation.toFixed(2),
                totalDepreciation.toFixed(2)
            ]);
            value -= depreciation;
            life -= 1;
        }

The updated code with all of the improvements up to now, is found at https://jsfiddle.net/pmw57/4bmcfsjn/2/

Further work is required though before JSLint is satisfied with the state of the code, which follows in the next post.

[Paul_Wilkins]

The next thing that JSLint is concerned about in your code is the unused Validating function. It's right, that function isn't used at all.

Validating

We could have the event handler call validating instead, but that's tying ti too much to what is happening, instead of why.

More appropriately we can have the showTable() function call the Validating() function instead. I know that JSLint will complain if a call is made to a function that's lower down in the code, so moving the Validating() function up until it's above the showTable() function is the first thing to do:

    function Validating() {
        // ...
    }

    function showTable(evt) {
        // ...
    }

We can then call Validating giving it the form as an argument, and instead of having it call showTable(), we can have it return true instead.

    function Validating(form) {
        // ...
        if (Valid && Valid2) {
            return true;
        }
    }

    function showTable(evt) {
        var ...
        if (!Validating(form)) {
            return;
        }
        // ...
    }

We just now need the Validating() function to use form.elements to retrieve the form values:

        // if (document.depForm.pValue.value === "" || document.depForm.pLife.value === "") {
        if (form.elements.pValue.value === "" || form.elements.pLife.value === "") {
        // ...
        // if (isNaN(document.depForm.pValue.value) || isNaN(document.depForm.pLife.value)) {
        if (isNaN(form.elements.pValue.value) || isNaN(form.elements.pLife.value)) {

And the Validating() code works.

Improved validation

There are many ways to improve this code, but the best code is when no code is used at all.

The form fields can be told that they are required and that they are expected to be numbers:

    <input type="number" name="pValue" id="value" value="" required>
    <br>
    <input type="number" name="pLife" id="life" value="" required>

And now the form won't allow anything other than numbers to be entered.

We can also tell the form that the Show Table button is a submit button, which causes the form to show good error messages when you attempt to use it with empty fields:

    <input type="submit" name="showTable" value="Show Table" />

Of course, that means that we also need to tell the showTable() function to prevent the default behaviour of submitting the form, and the built-in HTML validation of your form now works. That Validating() function isn't needed any more.

    function showTable(evt) {
        // ...    
        evt.preventDefault();
        // if (!Validating(form)) {
        //     return;
        // }

And because that Validating() function is no longer needed, it can be deleted.

Conclusion

What is next with JSLint - there are no more issues with the code which brings an end to this current form of assistance.
The code that we're left with is found at https://jsfiddle.net/pmw57/4bmcfsjn/6/ and is much improved over what we began with, thanks to the helpful proddings from JSLint.

Here's the updated JavaScript code in full:

(function iife() {
    "use strict";

    function ComputeFV(evt) {
        var button = evt.target;
        var form = button.form;
        var myPV = parseFloat(form.elements.PV.value);
        var myRate = parseFloat(form.elements.Rate.value);
        var myYear = Number(form.elements.Year.value);
        var fv = myPV * Math.pow(1 + myRate, myYear);
        form.elements.FV.value = fv.toString();
    }

    function addNumberedRow(table, values) {
        var rowCount = table.rows.length;
        var row = table.insertRow(rowCount);
        [rowCount, ...values].forEach(function addCell(value, index) {
            var cell = row.insertCell(index);
            cell.innerHTML = value;
        });
    }

    function showTable(evt) {
        var button = evt.target;
        var form = button.form;
        var value = Number(form.elements.pValue.value);
        var life = Number(form.elements.pLife.value);
        var depreciation = value / life;
        var table = document.getElementById("depTable");
        var totalDepreciation = 0;

        evt.preventDefault();

        while (table.rows.length > 0) {
            table.deleteRow(0);
        }
        while (life > 0) {
            totalDepreciation += depreciation;
            addNumberedRow(table, [
                value.toFixed(2),
                depreciation.toFixed(2),
                totalDepreciation.toFixed(2)
            ]);
            value -= depreciation;
            life -= 1;
        }
    }

    var fvForm = document.querySelector("#fvForm");
    var fvBtn = fvForm.elements.btnCompute;
    fvBtn.addEventListener("click", ComputeFV);

    var depForm = document.querySelector("#depForm");
    var showTableBtn = depForm.elements.showTable;
    showTableBtn.addEventListener("click", showTable);
}());

[system]

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.