19
\$\begingroup\$

For a quick summary: I've created this internal web application, and I've hit a point where I can really see the mess I've made. I need some help separating the logic, the view, and the data.

More detail: Over the past few months, I've been doing all I can to learn more about JavaScript built web sites/applications. I've created the below code, and it seems as though any additions are just ruining the entire thing. This is the fourth time I've started from scratch on this, and I can't get a product I really like.

Now, it works as it should, I just don't like the way it's built. I tried using Angular.js, but that was over-kill for this single page app (plus working with the routing was nightmarish). Now I've just created this mess of a main.js file, and it needs refactoring.

It'd be great if we could avoid suggesting tools requiring node.js.

index.html

<!DOCTYPE html>
<html>
    <head>
        <!-- Basic Page Needs
        –––––––––––––––––––––––––––––––––––––––––––––––––– -->
        <meta charset="utf-8">
        <title>Pies</title>
        <meta name="description" content="">
        <meta name="author" content="">

        <!-- Mobile Specific Metas
        –––––––––––––––––––––––––––––––––––––––––––––––––– -->
        <meta name="viewport" content="width=device-width, initial-scale=1">

        <!-- FONT
        –––––––––––––––––––––––––––––––––––––––––––––––––– -->
        <link href="//fonts.googleapis.com/css?family=Raleway:400,300,600" rel="stylesheet" type="text/css">

        <!-- SCRIPTS
        –––––––––––––––––––––––––––––––––––––––––––––––––– -->
        <script src="//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.js"></script>
        <script src="scripts/main.js"></script>

        <!-- CSS
        –––––––––––––––––––––––––––––––––––––––––––––––––– -->
        <link rel="stylesheet" href="css/normalize.css">
        <link rel="stylesheet" href="css/skeleton.css">
        <link rel="stylesheet" href="css/main.css">
    </head>
    <body>
        <div class="container">
            <header class="row">
                <div class="twelve columns">
                    <h1>Pies</h1>
                </div>
            </header>
            <div class="row">
                <div class="six columns" id="newOrderContainer">
                    <h2>New Order?</h2>
                    <span class="response success">Successfully created!</span>
                    <span class="response fail">Something went wrong, try again later.</span>
                    <form id="newOrderForm">
                        <label for="customerName">Customer name</label>
                        <input class="u-full-width" type="text" id="customerName">
                        <label for="dueDate">Due date (optional)</label>
                        <input type="date" id="dueDate"/>
                        <div class="flavorSelector u-full-width"></div>
                    </form>
                </div>
                <div class="six columns">
                    <h2>Payments</h2>
                </div>
            </div>
            <div class="row">
                <div class="twelve columns" id="ordersContainer">
                    <h2>Orders</h2>
                    <span class="response success">Successfully paid!</span>
                    <span class="response fail">Something went wrong, try again later.</span>
                    <div id="ordersTableContainer">
                        <table class="u-full-width">
                            <thead>
                                <tr>
                                    <th>Name</th>
                                    <th>Priority</th>
                                    <th>Flavor</th>
                                    <th>Payment</th>
                                </tr>
                            </thead>
                            <tbody></tbody>
                        </table>
                    </div>
                </div>
            </div>
            <div class="row">
                <div class="u-full-width">
                    <h2>Settings</h2>
                </div>
            </div>
            <div class="row">
                <div class="four columns" id="newFlavorContainer">
                    <h3>New Flavor</h3>
                    <span class="response success">Successfully added!</span>
                    <span class="response fail">Something went wrong, try again later.</span>
                    <form id="newFlavorForm">
                        <label for="newFlavor">Flavor</label>
                        <input class="u-full-width" type="text" placeholder="Apple, pecan, etc." id="newFlavor">
                        <div class="button" style="display: block;">
                            Add New Flavor
                        </div>
                    </form>
                </div>
                <div class="eight columns" id="flavorEditContainer">
                    <h3>Existing Flavors</h3>
                    <div id="flavorsTableContainer">
                        <table class="u-full-width">
                            <thead>
                                <tr>
                                    <th>Flavor</th>
                                    <th>Delete</th>
                                </tr>
                            </thead>
                            <tbody></tbody>
                        </table>
                    </div>
                </div>
            </div>
        </div>
    </body>
</html>

There's our main page. I've used the Skeleton CSS boilerplate.

main.js

$(document).ready(function() {
    $.get("scripts/appdata.json", function(data) {
        $(".flavorSelector").each(function() {
            for (flavor in data.flavors) {
                $(this).append('<div class="button" style="width: 60%">' + flavor + '</div>');
            }
        });
        for (order in data.orders) {
            var details = data.orders[order];
            if (details.paid === false) {
                $("#ordersTableContainer tbody").append(newOrderRow(order, details));
            }
        }
        for (flavor in data.flavors) {
            $("#flavorsTableContainer tbody").append(newFlavorRow(flavor));
        }
        $("#newOrderForm div.button").click(function() {
            var pdata = {
                "action" : "newOrder",
                "name" : $("#customerName").val(),
                "due" : $("#dueDate").val(),
                "flavor" : $(this).text()
            };
            $.post("scripts/server.php", pdata, function(data) {
                if (data != true) {
                    $("#newOrderContainer > span.fail").show().delay(5000).fadeOut(600, function() {
                        $(this).hide();
                    });
                } else {
                    $("#customerName").val("");
                    $("#dueDate").val("");
                    fetchOrders();
                    $("#newOrderContainer > span.success").show().delay(5000).fadeOut(600, function() {
                        $(this).hide();
                    });
                }
            });
        });
        $(".payment-form div.button").click(function() {
            var pdata = {
                "action" : "payOrder",
                "hash" : $(this).parents("tr").data("hash"),
                "paid" : $(this).siblings("input").val()
            };
            $.post("scripts/server.php", pdata, function(data) {
                if (data != true) {
                    $("#ordersContainer > span.fail").show().delay(5000).fadeOut(600, function() {
                        $(this).hide();
                    });
                } else {
                    $("tr[data-hash=" + pdata["hash"] + "]").fadeOut(600, function() {
                        $(this).remove();
                    }).delay(1000);
                    $("#ordersContainer > span.success").show().delay(5000).fadeOut(600, function() {
                        $(this).hide();
                    });
                }
            });
        });
    }, 'json');
    $("#newFlavorForm div.button").click(function() {
        var pdata = {
            "action" : "newFlavor",
            "flavor" : $("#newFlavorForm input[type=text]").val()
        };
        $.post("scripts/server.php", pdata, function(data) {
            if (data != true) {
                $("#newFlavorContainer > span.fail").show().delay(5000).fadeOut(600, function() {
                    $(this).hide();
                });
            } else {
                $("#newFlavorForm input[type=text]").val("");
                $("#newFlavorContainer > span.success").show().delay(5000).fadeOut(600, function() {
                    $(this).hide();
                });
            }
        });
    });
});
function newOrderRow(hash, data) {
    var name = data.name, flavor = data.flavor, paymentForm = "<form class='payment-form'><input type='text'/><div class='button paid'>Paid</div></form>";
    var priority = "<div class='priority' style='background: " + getPriority(Math.floor(Date.now() / 1000), data.made, data.due) + "'></div>";
    return "<tr data-hash='" + hash + "'><td>" + name + "</td><td>" + priority + "</td><td>" + flavor + "</td><td>" + paymentForm + "</td></tr>";
}

function newFlavorRow(flavor) {
    return "<tr><td><input type='text' value='" + flavor + "'/></td><td><span>Delete</span></td></tr>";
}

function getPriority(now, made, due) {
    var colors = ["#A30E0E", "#FF9401", "#6FBF0D"];
    var marks = [172800, 64800, 0];
    var elapsed = now - made;
    if (due == "") {
        for (var i = 0; i < marks.length; i++) {
            if (elapsed > marks[i]) {
                return colors[i];
            }
        }
    }
    var until = due - now;
    var i = 0;
    colors.reverse();
    for (var i = 0; i < marks.length; i++) {
        console.log(until, ">", marks[i], i);
        if (until > marks[i]) {
            return colors[i];
        }
    }
}

I know, it's bad. Everything is mixed together, and I don't know what to do! Suggestions for architectures would be great, but if you could help me fit this into some framework, then would be great too.

Right now, I've got the data stored in a JSON file. I'd prefer not to have it in an RDBMS like MySQL, but I've never worked with anything else so I'm open to suggestions!

The data I have is looking like this:

{
    "flavors": {
        "Berry": "",
        "Apple": "",
        "Pecan": ""
    },
    "orders": {
        "43d133ecaed389cf527c93117fc29969": {
            "name": "Customer1",
            "flavor": "Berry",
            "made": 1421471493,
            "due": 1421884800,
            "paid": false
        },
        "4bb7e6668a2a63d32a7487267128d406": {
            "name": "Customer2",
            "flavor": "Pecan",
            "made": 1421471572,
            "due": 1421884800,
            "paid": false
        }
    }
}

I had some data being paired with flavors, but I got rid of that feature.

Is there any chance I can turn this project into something scalable, fast, and modern?

\$\endgroup\$
  • 6
    \$\begingroup\$ Bad code, great title - awesome post! \$\endgroup\$ – Mathieu Guindon Jan 17 '15 at 22:52
  • \$\begingroup\$ Could you provide a fiddle? \$\endgroup\$ – Chrillewoodz Jan 18 '15 at 11:07
  • \$\begingroup\$ @Chrillewoodz The entire functionality is based on Ajax, so if I did you would only see static HTML. I'm not sure this would help? \$\endgroup\$ – Alex L Jan 18 '15 at 16:44
  • \$\begingroup\$ Ah true, didn't think about that. \$\endgroup\$ – Chrillewoodz Jan 18 '15 at 19:21
  • \$\begingroup\$ @AlexL You could use a mocking library like Sinon.JS to write some simple functions to mock various responses for live testing, but that's admittedly a bit of work for a code review. But it's superb for unit testing. \$\endgroup\$ – David Harkness Jan 26 '15 at 4:14
13
+150
\$\begingroup\$

First of all I'd like to congratulate you on the HTML part, that one looks clean and takes almost all best practices into consideration. I say 'almost' since ... yeah well ... these days people argue you should put script tags before the </body> tag. This to avoid http stalling the reflow of the browser. Oh well, for simple applications leave it like that. If you want to scale up and add more libraries in the future, you might consider moving the scripts to the bottom.

Next one then, the JavaScript part. If you say it works, well done! You say it looks ugly ... ? Do you also know why? Let me sum that up for you just to make sure we're on the same page:

  • logic mixed with strings is a "nono" if you want to write beautiful code => configurable strings
  • the templating is kinda hard-coded into the logic => templating system
  • the use of globally defined functions => closure
  • the amount of iterations (not sure if I can reduce them, we'll see along the way) => best practices in DOM appending
  • no function describes what the "main" part is (this becomes essential once you scale up) => modular approach

You don't want to use some additional libraries for this piece of code? I totally agree! You sound like a good decision maker, now you need a little push in the right direction.

So ... I've been spreading my logic here and there over stackoverflow/codereview and I believe it will help you too. Please read them as I'm not going to copy/paste the whole idea again. I will provide refactored code and the extra information I'm sure you can take that from some of those answers I've linked.

I use Re-Sharper for JavaScript and I like it "green" (read: jshint valid) so let me tell you what goes wrong even though your code "works":

  • ln 24 & 45: Declaration hides parameter data from outer scope
  • ln 32: Use of an implicitly declared global variable 'fetchOrders' (assuming this is a false positive)
  • ln 95 102 104: Duplicate declaration
  • ln 102: Value assigned is not used in any execution path
  • ln 110: Not all code paths return a value

So this is how I do it. Take the time to compare the approach below with my previously posted answers. It's actually the same stuff over and over again. Once you get the hang of it, you'll notice the benefit of object literals and how to extend/configure YOUR OWN library.

window.DeliciousPie = (function ($, project) {

    // 1. CONFIGURATION
    var cfg = {
        cache: {
            container: '[data-component="orderpie"]',
            flavors: '.flavorSelector',
            flavorsTable: '#flavorsTableContainer tbody',
            flavorForm: '#newFlavorForm',
            flavorFormInputs: 'input[type=text]',
            flavorSuccess: '#newFlavorContainer > span.success',
            ordersTable: '#ordersTableContainer tbody',
            orderForm: '#newOrderForm',
            orderSuccess: '#newOrderContainer > span.success',
            orderFail: '#newOrderContainer > span.fail',
            dueDate: '#dueDate',
            customerName: '#customerName',
            paymentForm: '.payment-form',
            paymentSuccess: '#ordersContainer > span.success',
            paymentFail: '#ordersContainer > span.fail',
            formTarget: 'div.button'
        },
        data: {
            hash: 'hash'
        },
        events: {
            click: 'click'
        },
        tpl: {
            flavor: '<div class="button" style="width: 60%">{{flavor}}</div>',
            paymentForm: '<form class="payment-form"><input type="text"/><div class="button paid">Paid</div></form>',
            priority: '<div class="priority" style="background: "{{priority}}"></div>',
            orderRow: '<tr data-hash="{{hash}}"><td>{{name}}</td><td>{{priority}}</td><td>{{flavor}}</td><td>{{paymentForm}}</td></tr>',
            flavorRow: '<tr><td><input type="text" value="{{flavor}}"/></td><td><span>Delete</span></td></tr>'
        },
        ajaxOptions: {
            get: {
                url: 'scripts/appdata.json',
                dataType: 'json'
            },
            post: {
                flavor: {
                    url: 'scripts/server.php',
                    data: {
                        action: 'newFlavor'
                    }
                },
                order: {
                    url: 'scripts/server.php',
                    data: {
                        action: 'newOrder'
                    }
                },
                pay: {
                    url: 'scripts/server.php',
                    data: {
                        action: 'payOrder'
                    }
                }
            }
        },
        priorityOptions: {
            colors: ['#A30E0E', '#FF9401', '#6FBF0D'],
            marks: [172800, 64800, 0]
        }
    };

    // 2. ADDITIONAL FUNCTIONS
    /**
     * @description Render html template with json data
     * @see handlebars or mustache if you need more advanced functionality
     * @param {Object} obj
     * @param {String} template : html template with {{keys}} matching the object
     * @return {String} template : the template string replaced by key:value pairs from the object
     */
    function renderTemplate(obj, template) {
        var tempKey, reg, key;

        for (key in obj) {
            if (obj.hasOwnProperty(key)) {
                tempKey = String("{{" + key + "}}");
                reg = new RegExp(tempKey, "g");
                template = template.replace(reg, obj[key]);
            }
        }

        return template;
    }

    // 3. COMPONENT OBJECT
    project.OrderPie = {

        version: 0.1,

        init: function () {
            this.cacheItems();

            if (this.container.length) {
                this.getData();
                this.bindEvents();
            }
        },
        cacheItems: function () {
            var cache = cfg.cache;

            this.container = $(cache.container);

            this.flavors = $(cache.flavors);
            this.flavorsTable = $(cache.flavorsTable);
            this.flavorForm = $(cache.flavorForm);
            this.flavorFormInputs = this.flavorForm.find(cache.flavorFormInputs);
            this.flavorSuccess = $(cache.flavorSuccess);
            this.flavorFail = $(cache.flavorFail);

            this.ordersTable = $(cache.ordersTable);
            this.orderForm = $(cache.orderForm);
            this.dueDate = $(cache.dueDate);
            this.customerName = $(cache.customerName);
            this.orderSuccess = $(cache.orderSuccess);
            this.orderFail = $(cache.orderFail);

            this.paymentForm = $(cache.paymentForm);
            this.paymentSuccess = $(cache.paymentSuccess);
            this.paymentFail = $(cache.paymentFail);
        },
        bindEvents: function () {
            var self = this,
                cache = cfg.cache,
                data = cfg.data,
                events = cfg.events,
                ajaxOptions = cfg.ajaxOptions;

            this.flavorForm.on(events.click, cache.formTarget, function () {
                var options = $.extend({}, ajaxOptions.post.flavor, {
                    data: {
                        flavor: self.flavorFormInputs.val()
                    }
                });

                $.ajax(options).done(function (flavorData) {
                    if (flavorData) {
                        self.flavorFormInputs.val('');
                        self.flavorSuccess.show().delay(5000).hide(600);
                    }
                }).fail(function () {
                    self.flavorFail.show().delay(5000).hide(600);
                });
            });

            this.orderForm.on(events.click, cache.formTarget, function () {
                var options = $.extend({}, ajaxOptions.post.order, {
                    data: {
                        name: self.customerName.val(),
                        due: self.dueDate.val(),
                        flavor: $(this).text()
                    }
                });

                $.ajax(options).done(function (orderData) {
                    if (orderData) {
                        self.customerName.val('');
                        self.dueDate.val('');
                        self.fetchOrders();
                        self.orderSuccess.show().delay(5000).hide(600);
                    }
                }).fail(function () {
                    self.orderFail.show().delay(5000).hide(600);
                });
            });

            this.paymentForm.on(events.click, cache.formTarget, function () {
                var options = $.extend({}, ajaxOptions.post.order, {
                    data: {
                        hash: $(this).closest('tr').data(data.hash),
                        paid: $(this).siblings('input').val()
                    }
                });

                $.ajax(options).done(function (paymentData) {
                    if (paymentData) {
                        $('[data-hash="' + options.data.hash + '"]').hide(600).delay(1000).remove();

                        self.paymentSuccess.show().delay(5000).hide(600);
                    }
                }).fail(function () {
                    self.paymentFail.show().delay(5000).hide(600);
                });
            });
        },
        getData: function () {
            var self = this;

            $.ajax(cfg.ajaxOptions.get).done(function (data) {
                if (data.hasOwnProperty('flavors')) {
                    self.setFlavors(data.flavors);
                }

                if (data.hasOwnProperty('orders')) {
                    self.setOrders(data.orders);
                }
            });
        },
        setFlavors: function (dataFlavors) {
            var tpl = cfg.tpl.flavor,
                rows = [],
                arr = [];

            this.flavors.each(function () {
                for (var flavor in dataFlavors) {
                    arr.push(renderTemplate(flavor, tpl));
                    rows.push(this.addFlavorRow(flavor));
                }

                $(this).append(arr);
                this.flavorsTable.append(rows);
            });
        },
        setOrders: function (dataOrders) {
            var details,
                rows = [];

            for (var order in dataOrders) {
                if (dataOrders.hasOwnProperty(order)) {
                    details = dataOrders[order];
                    if (!details.paid) {
                        rows.push(this.addOrderRow(order, details));
                    }
                }
            }

            this.ordersTable.append(rows);
        },
        addOrderRow: function (hash, data) {
            var tplVars = $.extend({}, data, {
                paymentform: cfg.tpl.paymentForm,
                priority: getPriority(Math.floor(+(new Date) / 1000), data.made, data.due),
                hash: hash
            });

            return renderTemplate(tplVars, cfg.tpl.orderRow);
        },
        addFlavorRow: function (flavor) {
            return renderTemplate({flavor: flavor}, cfg.tpl.flavorRow);
        },
        getPriority: function (now, made, due) {
            var priorityOptions = cfg.priorityOptions,
                colors = priorityOptions.colors,
                marks = cfg.priorityOptions.marks,
                elapsed = now - made,
                until = due - now;

            if (!due) {
                for (var i = 0; i < marks.length; i++) {
                    if (elapsed <= marks[i]) {
                        continue;
                    }

                    return colors[i];
                }
            }

            colors.reverse();

            for (var j = 0; j < marks.length; j++) {
                console.log(until, ">", marks[j], j);
                if (until <= marks[j]) {
                    continue;
                }

                return colors[j];
            }
        },
        fetchOrders: function () {
            console.warn('not implemented function');
        }

    };

    // 4. GLOBALIZE NAMESPACE
    return project;

}(window.jQuery, window.DeliciousPie || {}));

Once this file is loaded, you can call DeliciousPie.OrderPie.init() on DOM ready and you're good to order some pie (or whatever it is :p)

What you gain with this approach:

  • configurable objects
  • extendable objects (multiple HTML classes, activated by JavaScript, with different config if needed)
  • separation of concerns
  • scalable/modular approach
  • event control
  • reflow optimization
  • memory optimization
  • better readability
  • an easy templating sytem for free (no additional libraries required ^^)
  • RESPEC(t) from your colleagues
  • I can invent some more, but all in all, quality code

1) Overhead

When a lot of components/modules are loaded from one file and let's say you have a lot of pages ... the overhead you create for undetected modules/components:

  • cfg variable => so try to keep strings in it and only extend cfg in a method
  • cacheItems() => depends on the speed of your selectors and sizzle
  • init() method checking the length of the container

So scalability wise this performs very well. Remember JavaScript in itself is really fast. It's the DOM that slows down quite a lot. For that reason it could be interesting to split-up the cacheItems.

2) Templating

The templating in my example is also not "ideal". It's very basic but also puts HTML into JavaScript and then you can argue that separation of concerns doesn't apply to this approach. Hence the whole script idea seen in Handlebars/Mustache which covers that. However, I would only take that approach if logic in templating is required {{if}}{{else}}{{/if}}. For string replacement only, keep it simple. Extra logics for the templates while looping can be done inside a specific function as well (ex: addOrderRow, addFlavorRow). Besides, you can always leave a comment <!-- js rendering --> inside your HTML as well ...

As suggested in the comments: you can create a hidden class or with a data- attribute and pick those chunks up.

Some additional reads:

If I find some time I'll try to test this as well. Probably you'll need to add the flavor data back in there for the templating sytem. And a data-component="orderpie" on the "main" wrapper to kick it in. I hope you are familiar with debugging tools. If not, at least I hope you'll learn a thing or 4. GL!

\$\endgroup\$
  • 1
    \$\begingroup\$ Nice - I'd take a step further, though, and add the templates as hidden elements in the markup. Say make a "template" class that hides those element and makes them easy to find. That'll keep the markup in one place, so you don't have to write HTML in you JS \$\endgroup\$ – Flambino Jan 22 '15 at 17:34
  • \$\begingroup\$ Yes, that would indeed be an even better approach. \$\endgroup\$ – Tim Vermaelen Jan 22 '15 at 19:15
  • \$\begingroup\$ Wow! I really like what you've done here. Talk about refactoring! I appreciate the time you must have put into this, and it's easy to read and interpret. I'll have to work with it and see what I can do. I do have a couple questions though, 1) imagine this app gets bigger, much bigger, even if I separated the different parts into other files, how well would this scale? 2) what's your opinion on using a more versatile templating library? \$\endgroup\$ – Alex L Jan 23 '15 at 5:24
  • \$\begingroup\$ Thanks for that template chooser link, didn't know about that one. Although you might have noticed certain option combinations yield no results. I've updated my answer to your questions. \$\endgroup\$ – Tim Vermaelen Jan 23 '15 at 14:03
  • \$\begingroup\$ Great job and very clean. There are two bugs in setFlavors: you mix this as the jQuery flavors selector elements and the OrderPie module, and you're missing a call to hasOwnProperty (unless there are cases you recommend omitting it). \$\endgroup\$ – David Harkness Jan 26 '15 at 3:44

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.