current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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?

share|improve this question

2 Answers 2

up vote 2 down vote accepted

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', '');
    };
}
share|improve this answer
    
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? –  WMRKameleon Nov 3 '13 at 13:25
    
No I haven't tested this code as a whole but those lines will definitely work. jsfiddle.net/N5wQw/1 –  Stuart Nov 3 '13 at 13:35
    
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. –  WMRKameleon Nov 3 '13 at 13:40
    
I think you need to change E('placeHolder') to E('shoppingList')? I have edited the code above to make it match your OP –  Stuart Nov 3 '13 at 13:43
    
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? –  WMRKameleon Nov 3 '13 at 13:53
up vote 0 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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