I was wondering if this code can be improved

JavaScript
1

I was wondering if I could get a review of this bit of code I wrote up today. Basically I’m grabbing some data from a mysql table through php storing it in a json object and then sorting by rows and columns and displaying as necessary. Everything is working fine for me as I can tell but I’d like some input as I’m not that great at javascript and am just looking for some feed back. The first section of code is the script. Please note that I am just pasting the returned json object at the bottom of the post. Just in case you would like to give it a whirl.


<?
$test = file_get_contents('data.json');

?>

<!doctype html>
<html>
    <script src="//ajax.googleapis.com/ajax/libs/jquery/1.10.1/jquery.min.js"></script>
    <script>
        $(function() {
            var allCabinets = <?= $test ?>;
            var current_height = 0;
            var current_width = 0;
            var uniqueHeights = getDistinct(allCabinets);
            var defaultHeight = buildCabinets(allCabinets,uniqueHeights[current_height]);
            var uniqueWidths = getDistinctColumns(defaultHeight);
            var display = buildCabinets(allCabinets,uniqueHeights[current_height],uniqueWidths[current_width]);            
            
    createTable(display);
    function increaseHeight(){
                var maxHeight = uniqueHeights.length;
                if(current_height < maxHeight-1){
                    current_height=current_height+1;
                    m = buildCabinets(allCabinets,uniqueHeights[current_height]);
                    uniqueWidths = getDistinctColumns(m);
                    current_width = 0;
                    display = buildCabinets(allCabinets,uniqueHeights[current_height], uniqueWidths[current_width]);
                    console.log(display);
                    createTable(display);
                }else{
                    display = buildCabinets(allCabinets,uniqueHeights[current_height], uniqueWidths[current_width]);
                    console.log(display);
                    createTable(display);
                }
                
            }
            function decreaseHeight(){
                if(current_height > 0){
                    current_height=current_height-1;
                    m = buildCabinets(allCabinets,uniqueHeights[current_height]);
                    uniqueWidths = getDistinctColumns(m);
                    current_width = 0;
                    display = buildCabinets(allCabinets,uniqueHeights[current_height], uniqueWidths[current_width]);
                    console.log(display);
                    createTable(display);
                }else{
                    display = buildCabinets(allCabinets,uniqueHeights[current_height], uniqueWidths[current_width]);
                    console.log(display);
                    createTable(display);
                }
                
            }
            
            function increaseWidth(){
                var maxWidth = uniqueWidths.length;
                if(current_width < maxWidth-1){
                    current_width=current_width+1;
                    m = buildCabinets(allCabinets,uniqueHeights[current_height]);
                    uniqueWidths = getDistinctColumns(m);
                    display = buildCabinets(allCabinets,uniqueHeights[current_height], uniqueWidths[current_width]);
                    console.log(display);
                    createTable(display);
                }else{
                    display = buildCabinets(allCabinets,uniqueHeights[current_height], uniqueWidths[current_width]);
                    console.log(display);
                    createTable(display);
                }
                
            }
            function decreaseWidth(){
                if(current_width > 0){
                    current_width=current_width-1;
                    m = buildCabinets(allCabinets,uniqueHeights[current_height]);
                    uniqueWidths = getDistinctColumns(m);
                    display = buildCabinets(allCabinets,uniqueHeights[current_height], uniqueWidths[current_width]);
                    console.log(display);
                    createTable(display);
                }else{
                    display = buildCabinets(allCabinets,uniqueHeights[current_height], uniqueWidths[current_width]);
                    console.log(display);
                    createTable(display);
                }
                
            }
            
            function buildCabinets(c, x, y) {
                var res = c.filter(function(el) {
                    return typeof y !== 'undefined' ? el.signRows == x && el.signColumns == y : el.signRows == x;
                });
                return res;
            }
            function getDistinctColumns(data){
                var arr = new Array();
                $.each(data, function(i, sign){
                    if(jQuery.inArray(sign.signColumns,arr)===-1){
                        arr.push(sign.signColumns);
                    }
                });
                return arr.sort(function(a,b){return a-b});
            }
            function getDistinct(data) {
                var arr = new Array();
                $.each(data, function(i, sign) {
                    if (jQuery.inArray(sign.signRows, arr) === -1) {
                        arr.push(sign.signRows);
                    }
                });
                return arr.sort(function(a,b){return a-b});
            }

            function createTable(result) {
                var data = result;      
                $("#placeholder").html("");
                var tableHeader = "<table><thead><tr>";
                var tableBody = "<tbody>";
                var endTable = "</table>";

                tableHeader += "<th>ID</th><th>Pitch</th><th>Matrix</th></tr></thead>";
                $(tableHeader).appendTo("#placeholder");

                $(data).each(function(index,item){

                    tableBody += "<tr>";
                    tableBody +="<td>" + item['id'] + "</td>";
                    tableBody +="<td>" + item['signPitch'] + "</td>";
                    tableBody +="<td>" + item['signRows'] + " x " + item['signColumns'] + "</td>";
                    tableBody += "</tr>";

                });

                tableBody += "</tbody>";

                $(tableBody).appendTo("#placeholder");              
                $(endTable).appendTo("#placeholder");

            }   
            
            $('#increaseHeight').click(function(e){
                increaseHeight();
                $("#pixelsTall").val(display[0]['signRows']);
                $("#pixelsWide").val(display[0]['signColumns']);
                e.preventDefault();
            });
            $('#decreaseHeight').click(function(e){
                decreaseHeight();
                $("#pixelsTall").val(display[0]['signRows']);
                $("#pixelsWide").val(display[0]['signColumns']);
                e.preventDefault();
            });
            $('#increaseWidth').click(function(e){
                increaseWidth();
                $("#pixelsWide").val(display[0]['signColumns']);
                e.preventDefault();
            });
            $('#decreaseWidth').click(function(e){
                decreaseWidth();
                $("#pixelsWide").val(display[0]['signColumns']);
                e.preventDefault();
            });
        });
    </script>
						<fieldset class="one-third">
                                                    <legend>Find by Matrix</legend>
                                                    <input type="text" name="pixelsTall" id="pixelsTall" value=""/>
                                                    <button name="increase-matrix-pixel-tall" id="increaseHeight">+</button>
                                                    <button name="decrease-matrix-pixel-tall" id="decreaseHeight">-</button>
                                                    <label>Pixels Tall</label>
                                                    <div class="clear"></div>
                                                    <input type="text" name="pixelsWide" id="pixelsWide"/>
                                                    <button name="increase-matrix-pixel-wide" id="increaseWidth">+</button>
                                                    <button name="decrease-matrix-pixel-wide" id="decreaseWidth">-</button>
                                                    <label>Pixels Wide</label>
						</fieldset>
    <div id="placeholder"></div>
</html>



[{"id":"1","signRows":"32","signColumns":"96","signModel":"16","signPitch":"16"},{"id":"2","signRows":"32","signColumns":"112","signModel":"16","signPitch":"16"},{"id":"3","signRows":"32","signColumns":"128","signModel":"16","signPitch":"16"},{"id":"4","signRows":"32","signColumns":"144","signModel":"16","signPitch":"16"},{"id":"5","signRows":"32","signColumns":"160","signModel":"16","signPitch":"16"},{"id":"6","signRows":"48","signColumns":"96","signModel":"16","signPitch":"16"},{"id":"7","signRows":"48","signColumns":"112","signModel":"16","signPitch":"16"},{"id":"8","signRows":"48","signColumns":"128","signModel":"16","signPitch":"16"},{"id":"9","signRows":"48","signColumns":"144","signModel":"16","signPitch":"16"},{"id":"10","signRows":"48","signColumns":"160","signModel":"16","signPitch":"16"},{"id":"11","signRows":"64","signColumns":"96","signModel":"16","signPitch":"16"},{"id":"12","signRows":"64","signColumns":"112","signModel":"16","signPitch":"16"},{"id":"13","signRows":"64","signColumns":"128","signModel":"16","signPitch":"16"},{"id":"14","signRows":"64","signColumns":"144","signModel":"16","signPitch":"16"},{"id":"15","signRows":"64","signColumns":"160","signModel":"16","signPitch":"16"},{"id":"16","signRows":"80","signColumns":"224","signModel":"16","signPitch":"16"},{"id":"17","signRows":"16","signColumns":"80","signModel":"20","signPitch":"20"},{"id":"18","signRows":"16","signColumns":"96","signModel":"20","signPitch":"20"},{"id":"19","signRows":"16","signColumns":"112","signModel":"20","signPitch":"20"},{"id":"20","signRows":"16","signColumns":"128","signModel":"20","signPitch":"20"},{"id":"21","signRows":"16","signColumns":"144","signModel":"20","signPitch":"20"},{"id":"22","signRows":"16","signColumns":"160","signModel":"20","signPitch":"20"},{"id":"23","signRows":"24","signColumns":"80","signModel":"20","signPitch":"20"},{"id":"24","signRows":"24","signColumns":"96","signModel":"20","signPitch":"20"},{"id":"25","signRows":"24","signColumns":"112","signModel":"20","signPitch":"20"},{"id":"26","signRows":"24","signColumns":"128","signModel":"20","signPitch":"20"},{"id":"27","signRows":"24","signColumns":"144","signModel":"20","signPitch":"20"},{"id":"28","signRows":"24","signColumns":"160","signModel":"20","signPitch":"20"},{"id":"29","signRows":"32","signColumns":"80","signModel":"20","signPitch":"20"},{"id":"30","signRows":"32","signColumns":"96","signModel":"20","signPitch":"20"},{"id":"31","signRows":"32","signColumns":"112","signModel":"20","signPitch":"20"},{"id":"32","signRows":"32","signColumns":"128","signModel":"20","signPitch":"20"},{"id":"33","signRows":"32","signColumns":"144","signModel":"20","signPitch":"20"},{"id":"34","signRows":"32","signColumns":"160","signModel":"20","signPitch":"20"},{"id":"35","signRows":"48","signColumns":"96","signModel":"20","signPitch":"20"},{"id":"36","signRows":"48","signColumns":"112","signModel":"20","signPitch":"20"},{"id":"37","signRows":"48","signColumns":"128","signModel":"20","signPitch":"20"},{"id":"38","signRows":"48","signColumns":"144","signModel":"20","signPitch":"20"},{"id":"39","signRows":"48","signColumns":"160","signModel":"20","signPitch":"20"},{"id":"40","signRows":"64","signColumns":"96","signModel":"20","signPitch":"20"},{"id":"41","signRows":"64","signColumns":"112","signModel":"20","signPitch":"20"},{"id":"42","signRows":"64","signColumns":"128","signModel":"20","signPitch":"20"},{"id":"43","signRows":"64","signColumns":"144","signModel":"20","signPitch":"20"},{"id":"44","signRows":"64","signColumns":"160","signModel":"20","signPitch":"20"},{"id":"45","signRows":"16","signColumns":"80","signModel":"25","signPitch":"25"},{"id":"46","signRows":"16","signColumns":"96","signModel":"25","signPitch":"25"},{"id":"47","signRows":"16","signColumns":"112","signModel":"25","signPitch":"25"},{"id":"48","signRows":"16","signColumns":"128","signModel":"25","signPitch":"25"},{"id":"49","signRows":"16","signColumns":"144","signModel":"25","signPitch":"25"},{"id":"50","signRows":"16","signColumns":"160","signModel":"25","signPitch":"25"},{"id":"51","signRows":"32","signColumns":"96","signModel":"25","signPitch":"25"},{"id":"52","signRows":"32","signColumns":"112","signModel":"25","signPitch":"25"},{"id":"53","signRows":"32","signColumns":"128","signModel":"25","signPitch":"25"},{"id":"54","signRows":"32","signColumns":"144","signModel":"25","signPitch":"25"},{"id":"55","signRows":"32","signColumns":"160","signModel":"25","signPitch":"25"},{"id":"56","signRows":"48","signColumns":"96","signModel":"25","signPitch":"25"},{"id":"57","signRows":"48","signColumns":"112","signModel":"25","signPitch":"25"},{"id":"58","signRows":"48","signColumns":"128","signModel":"25","signPitch":"25"},{"id":"59","signRows":"48","signColumns":"144","signModel":"25","signPitch":"25"},{"id":"60","signRows":"48","signColumns":"160","signModel":"25","signPitch":"25"},{"id":"61","signRows":"64","signColumns":"128","signModel":"25","signPitch":"25"},{"id":"62","signRows":"16","signColumns":"64","signModel":"34","signPitch":"34"},{"id":"63","signRows":"16","signColumns":"80","signModel":"34","signPitch":"34"},{"id":"64","signRows":"16","signColumns":"96","signModel":"34","signPitch":"34"},{"id":"66","signRows":"24","signColumns":"64","signModel":"34","signPitch":"34"},{"id":"67","signRows":"24","signColumns":"80","signModel":"34","signPitch":"34"},{"id":"68","signRows":"24","signColumns":"96","signModel":"34","signPitch":"34"},{"id":"69","signRows":"32","signColumns":"64","signModel":"34","signPitch":"34"},{"id":"70","signRows":"32","signColumns":"80","signModel":"34","signPitch":"34"},{"id":"71","signRows":"32","signColumns":"96","signModel":"34","signPitch":"34"},{"id":"72","signRows":"48","signColumns":"64","signModel":"34","signPitch":"34"},{"id":"73","signRows":"48","signColumns":"80","signModel":"34","signPitch":"34"},{"id":"74","signRows":"48","signColumns":"96","signModel":"34","signPitch":"34"}]
2

Variables are currently individually declared - it’s preferred to define the variables of any particular scope as a single group, all from the one var statement.

The variable m is defined as a global variable, when there’s no need for it to be global. I would make good use of the already existing defaultHeight variable instead because that’s what it is, right?

Single letter variables? Those make for difficult to understand code unless they are well-understood single-letter vars such as i in a for loop. Even when using $.each() it’s preferred to use index as the function parameter instead of just the single letter.

The buildCabinets function requires some thinking about before you can properly understand the function.
Basically, it’s checking if an optional parameter is used or not. If it’s not used, it’s returning results without that parameter, and if that optional one is used, it’s making use of it too. It’s preferable to have a longer and easier to understand function, than to have a shorter but more opaque one, so a version of the function that may be easier to understand at a glance is something like the following:


function buildCabinets(cabinets, height, width) {
    return cabinets.filter(function (el) {
        var hasHeight = el.signRows === height,
            hasWidth = true;

        if (width) {
            hasWidth = el.signColumns === width;
        }
        return hasHeight && hasWidth;
    });
}

The filter function could also be condensed to the following:


var hasHeight = el.signRows === height,
    hasWidth = width && el.signColumns === width;

return hasHeight && hasWidth;

or even to this:


return (el.signRows === height) &&
    (width && el.signColumns === width);

but as for which is more readable and immediately understandable, that all depends on how well you can determine that the function is correctly doing its job, for which the initial function that sets hasWidth = true seems to achieve nicely.

Things like sorting numerically are useful to extract out, so that they may be also potentially used in other places.


function numericSort(a, b) {
    return a - b;
}
...
return arr.sort(numericSort);

The createTable function currently results in the following bad HTML structure.


<div id="placeholder">
    <table>
        <thead>...</thead>
    </table>
    <tbody>...</tbody>
</div>

What you should be doing instead, is to add both the header and the body to the table, and to then append that table to the placeholder.

This can be done in a very simple way, by moving off the thead and tbody parts to other functions, resulting in a createTable function that is simple and difficult to make mistakes with:


function createTable(data) {
    $("#placeholder").html(
        $('<table>')
            .append(createHead())
            .append(createBody(data))
    );
}

The createHead and createBody functions are no more complex either:


function createHead() {
    var content = '<tr><th>ID</th><th>Pitch</th><th>Matrix</th></tr>';
    return $('<thead>', { html: content});
}

function createBody(data) {
    var content = $.map(data, function (item) {
        return '<tr>' +
            '<td>' + item.id + '</td>' +
            '<td>' + item.signPitch + '</td>' +
            '<td>' + item.signRows + ' x ' + item.signColumns + '</td>' +
            '</tr>';
    });

    return $('<tbody>', {html: content});
}

The increaseHeight, decreaseHeight, increaseWidth and decreaseWidth functions have lots of duplication throughout them, so let’s apply some DRY principles to them.

After moving out common sections to a few separate functions, we can easily remove the else clauses resulting in the following:


function increaseHeight() {
    var maxHeight = uniqueHeights.length;
    if (current_height < maxHeight - 1) {
        current_height = current_height + 1;
        current_width = 0;
        updateDimensions();
    }
    showTable(current_height, current_width);
}

With a bit more effort, all reliance on a range of global variables could be reduced too.

How much of a review are you after? Running your code through a linting service such as jslint.com results in many other beneficial improvements too.

[list][]It’s better to use dot notation instead of array notation when referring to properties of an object.
[]Most code style guides recommend that formatting uses appropriate spaces for readability, between the function word and the opening parenthesis, after commas, and between the closing parenthesis and opening brace, and between operators.
[]Arrays are preferred to be declared as [] instead of new Array()
[]It’s preferred to define functions before they are used.
[/list]

3

Your are the flipping bomb.

4