2
\$\begingroup\$

I've got a JavaScript object and I am not sure how to implement the last part: adding up all of the prices and putting that in the last row of the table. Besides this question, I would also like to ask you to look at the code and advice me if I can make the code slimmer, less lines of code.

This is the code I would like reviewed:

    function ShoppingList(){
    this.items = [];
    this.setItem = function(product, price){
        var item = new Item(product, price);
        this.items.push(item);
    };
    this.render = function(){
        var placeHolder = document.getElementById("shoppingList");
        placeHolder.innerHTML = "";
        var messageDiv = document.getElementById("message");
        messageDiv.innerHTML = "";

        var tr = document.createElement("tr");
        tr.id = "header";

        var thProduct = document.createElement("th");
        thProduct.innerHTML = "Product";
        tr.appendChild(thProduct);

        var thPrice = document.createElement("th");
        thPrice.innerHTML = "Prijs";
        tr.appendChild(thPrice);

        var thDel = document.createElement("th");
        thDel.innerHTML = "Verwijder";
        tr.appendChild(thDel);

        placeHolder.appendChild(tr);

        for(var i = 0; i < this.items.length; i++){
            var tr = document.createElement("tr");
            tr.id = i;

            var tdProduct = document.createElement("td");
            tdProduct.innerHTML = this.items[i].product;
            tr.appendChild(tdProduct);

            var tdPrice = document.createElement("td");
            tdPrice.innerHTML = Number(this.items[i].price);
            tr.appendChild(tdPrice);

            var tdDel = document.createElement("td");
            tdDel.innerHTML = "Verwijder";
            tdDel.addEventListener("click", delItem, false);
            tr.appendChild(tdDel);

            placeHolder.appendChild(tr);
        }
    };
}

So, in short: how can I write a new table row to the table with the total price that updates when table rows are added or removed and how can I slim down my code?

\$\endgroup\$
2
\$\begingroup\$

I usually make some simple helper functions to simplify and shorten the code building html. (As your programme gets more complicated you can expand these to suit your needs, or if you prefer, switch to a library like jquery.) In the version below I have also added a few lines to add a total row to the table. However, think about whether you need to rewrite the entire table each time it changes, or could simply add or delete individual rows. If the latter, you would want something like addRow, deleteRow and updateTotal methods.

function build(parent, type, innerHTML) {
    // Make a new element, optionally fill it with html, append it to the parent,
    // and return it.
    var el = document.createElement(type);
    if (innerHTML) {
        el.innerHTML = innerHTML;
    }
    parent.appendChild(el);
    return el;
}

function E(id) {
    // This function has no purpose except to save typing
    return document.getElementById(id);
}

function ShoppingList() {
    var totalCell;
    this.items = [];
    this.setItem = function (product, price) {
        var item = new Item(product, price);
        this.items.push(item);
    };
    this.render = function () {
        E('shoppingList').innerHTML = "";
        E('message').innerHTML = "";
        var tr = build(E('shoppingList'), 'tr');
        tr.id = 'header';
        build(tr, 'th', 'Product');
        build(tr, 'th', 'Prijs');
        build(tr, 'th', 'Verwijder');
        var totalPrice = 0;
        for (var i = 0; i < this.items.length; i++) {
            tr = build(E('shoppingList'), 'tr');
            tr.id = i;
            build(tr, 'td', this.items[i].product);
            build(tr, 'td', this.items[i].price);
            build(tr, 'td', 'Verwijder').addEventListener('click', delItem, false);
            totalPrice += this.items[i].price;
        }
        tr = build(E('shoppingList'), 'tr');
        build(tr, 'td', 'Total');
        totalCell = build(tr, 'td', totalPrice);
        build(tr, 'td', '');
    };
}
\$\endgroup\$
9
  • \$\begingroup\$ I have looked at your code line by line and I assume there is one thing going to be wrong, I am not sure though. Setting E('placeHolder').innerHTML = ""; and E('message').innerHTML = ""; going to cause an error to show up? \$\endgroup\$ Nov 3 '13 at 13:25
  • \$\begingroup\$ No I haven't tested this code as a whole but those lines will definitely work. jsfiddle.net/N5wQw/1 \$\endgroup\$
    – Stuart
    Nov 3 '13 at 13:35
  • \$\begingroup\$ Okay, your jsFiddle works like a charm indeed, but my developer tools in Chrome point out that "Uncaught TypeError: Cannot set property 'innerHTML' of null" So I am not quite sure why Chrome gives me this error, because the two div's certainly exist and such. \$\endgroup\$ Nov 3 '13 at 13:40
  • \$\begingroup\$ I think you need to change E('placeHolder') to E('shoppingList')? I have edited the code above to make it match your OP \$\endgroup\$
    – Stuart
    Nov 3 '13 at 13:43
  • \$\begingroup\$ Ah, thank you, I didn't see it had to be the element shoppingList instead of placeHolder! I am now trying to test it, I ran into one bug, being that if I add two products of price 1, it outputs a total price of 011, I am not sure how to solve that problem? \$\endgroup\$ Nov 3 '13 at 13:53
0
\$\begingroup\$

An alternative to @stuart's solution is to use a javascript template engine, like handlebars.

As a rule of thumb, I think that as soon as you create a render function, you should ask yourself if such a template engine would be useful. Template engines allow you to render html in a declarative way, which is often much more readable.

There is plenty of existing engines, check this stackoverflow question for more insight.

\$\endgroup\$

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.