5
\$\begingroup\$

Task:

Draw a grid with a given number of rows and columns. Each cell can be any color. The same grid should also be updated at a predetermined time interval. The grid should cover the entire visible portion of the page in the browser. Add a context menu with which you can change the parameters of the lattice.

I would really like to hear feedback on the code: syntax, logic and general any comments and tips.

I'm a young programmer so I want to hear real reviews for further development.

<!DOCTYPE HTML>
<html style="height:100%">
<head>
<meta charset="utf-8">
<title></title>
<link href="css/common.css" rel="stylesheet" type="text/css">
<script type="text/javascript" src="js/grid.js"></script>

<script type="text/javascript">

    window.onload = function () {

        var grid = new Grid ("20",              // rows
                              "6",              // columns
                              "2000");          // update grid interval

    }

</script>

</head>
<body style="margin:0; height:100%">
</body>
</html>

JS:

var interval = 0;
var stopUpdateGrid = false;

function Grid(numRows, numColumns, updateInterval) {

    this.numRows = numRows;
    this.numColumns = numColumns;
    this.updateInterval = updateInterval;

    var clientWidth = document.body.clientWidth;
    var clientHeight = document.body.clientHeight;

    (function createGrid(rows, columns) {

        var container = document.createElement("div");
        document.body.appendChild(container);
        container.style.width = clientWidth + "px";
        container.style.height = clientHeight + "px";
        container.setAttribute("id", "container");

        for (var i = 1; i <= rows * columns; i++) {
            var div = document.createElement("div");
            container.appendChild(div);
            div.style.width = (clientWidth - rows - 1) / rows + "px";
            div.style.height = (clientHeight - columns - 1) / columns + "px";
            div.style.cssFloat = "left";
            div.style.margin = "-1px 0 0 -1px";
            div.style.border = "1px solid #000";
            var n = randomColors();
            div.style.backgroundColor = "#" + n;
        }
    })(this.numRows, this.numColumns);

    function randomColors() {
        var min = 0;
        var max = 15;
        var colors = '';

        for (var i = 1; i <= 6; i++) {
            var rand = min - 0.5 + Math.random() * (max - min + 1);
            rand = Math.round(rand);
            rand = parseInt(rand, 10).toString(16);
            colors = colors + rand;
        }
        return colors;
    }

    (function updateColorsGrid(updInterval, rows, columns) {
        if (interval) {
            clearInterval(interval);
        }
        interval = setInterval(function() {
            if (stopUpdateGrid === false) {
                var container = document.getElementById("container");
                document.body.removeChild(container);
                var grid = new Grid(rows, columns, updInterval);
            }
        }, updInterval);
    })(this.updateInterval, this.numRows, this.numColumns);

    document.onkeyup = function(e) {
        e = e || window.event;
        if (e.keyCode === 13) {
            stopUpdateGrid = true;
            changeGridOptions();
        }
        return false;
    };

    (function createNotification() {
        var container = document.getElementById("container");
        var notificationContainer = document.createElement("div");
        container.appendChild(notificationContainer);
        notificationContainer.style.width = "500px";
        notificationContainer.style.height = "50px";
        notificationContainer.innerHTML = "Press Enter to show menu change!!!";
        notificationContainer.style.color = "#fff";
        notificationContainer.style.fontSize = "30px";
        notificationContainer.style.position = "absolute";
    })();

    function changeGridOptions() {
        var container = document.getElementById("container");
        var changeOptionsForm = document.createElement("form");
        container.appendChild(changeOptionsForm);
        changeOptionsForm.style.width = "200px";
        changeOptionsForm.style.height = "200px";
        changeOptionsForm.setAttribute("id", "changeOptionsForm");
        changeOptionsForm.style.border = "2px solid #fff";
        changeOptionsForm.style.backgroundColor = "#808080";
        changeOptionsForm.style.position = "absolute";
        changeOptionsForm.style.left = clientWidth - 220 + "px";
        changeOptionsForm.style.top = "15px";

        var changeRowsText = document.createElement("div");
        changeOptionsForm.appendChild(changeRowsText);
        changeRowsText.style.width = "50px";
        changeRowsText.style.height = "20px";
        changeRowsText.style.margin = "10px";
        changeRowsText.innerHTML = "Rows:";
        changeRowsText.style.cssFloat = "left";

        var changeRows = document.createElement("input");
        changeOptionsForm.appendChild(changeRows);
        changeRows.type = "text";
        changeRows.style.width = "100px";
        changeRows.style.height = "20px";
        changeRows.style.border = "2px solid #000";
        changeRows.style.margin = "10px";
        changeRows.style.cssFloat = "left";

        var changeColumnsText = document.createElement("div");
        changeOptionsForm.appendChild(changeColumnsText);
        changeColumnsText.style.width = "50px";
        changeColumnsText.style.height = "20px";
        changeColumnsText.style.margin = "10px";
        changeColumnsText.innerHTML = "Columns:";
        changeColumnsText.style.cssFloat = "left";

        var changeColumns = document.createElement("input");
        changeOptionsForm.appendChild(changeColumns);
        changeColumns.type = "text";
        changeColumns.style.width = "100px";
        changeColumns.style.height = "20px";
        changeColumns.style.border = "2px solid #000";
        changeColumns.style.margin = "10px";
        changeColumnsText.style.cssFloat = "left";

        var changeDelayUpdateGridText = document.createElement("div");
        changeOptionsForm.appendChild(changeDelayUpdateGridText);
        changeDelayUpdateGridText.style.width = "50px";
        changeDelayUpdateGridText.style.height = "20px";
        changeDelayUpdateGridText.style.margin = "10px";
        changeDelayUpdateGridText.innerHTML = "Delay Update:";
        changeDelayUpdateGridText.style.cssFloat = "left";

        var changeDelayUpdateGrid = document.createElement("input");
        changeOptionsForm.appendChild(changeDelayUpdateGrid);
        changeDelayUpdateGrid.type = "text";
        changeDelayUpdateGrid.style.width = "100px";
        changeDelayUpdateGrid.style.height = "20px";
        changeDelayUpdateGrid.style.border = "2px solid #000";
        changeDelayUpdateGrid.style.margin = "10px";
        changeDelayUpdateGrid.style.cssFloat = "left";

        var changeButton = document.createElement("input");
        changeOptionsForm.appendChild(changeButton);
        changeButton.type = "button";
        changeButton.value = "change";
        changeButton.style.width = "100px";
        changeButton.style.height = "25px";
        changeButton.style.margin = "20px 50px";
        changeButton.style.cssFloat = "left";

        changeButton.onclick = function() {
            numRows = changeRows.value || numRows;
            numColumns = changeColumns.value || numColumns;
            updateInterval = changeDelayUpdateGrid.value || updateInterval;
            changeOptionsForm.style.display = "none";
            stopUpdateGrid = false;
            document.body.removeChild(container);
            var grid = new Grid(numRows, numColumns, updateInterval);
        };
    }
}
\$\endgroup\$
3
  • 3
    \$\begingroup\$ Please leave the formatting alone. We are trying to make the question more readable for people who want to answer can understand the question. \$\endgroup\$
    – Jeff Vanzella
    Mar 6, 2014 at 23:11
  • \$\begingroup\$ What are you going to do if the user resizes their browser \$\endgroup\$
    – megawac
    Mar 6, 2014 at 23:19
  • \$\begingroup\$ I do not know why, but I never thought of that! then probably block "container" need to make 100%! \$\endgroup\$
    – sparcmen
    Mar 6, 2014 at 23:29

2 Answers 2

Reset to default
5
\$\begingroup\$

Just a couple of things that I really feel strongly about (didn't work line for line through your code though).

Seperation of logic and presentation

Yes, there is a .style object, however this does not mean you should use it as the primary way to style your objects. Instead just apply the corresponding class (either through the classList object or the backwards compatible class property), and only the really dynamic things (random background colour for example) should indeed be done through Javascript.

Always use addEventListener

In a simple stand alone case it doesn't matter, but at some point later you often end up regretting using things like .onclick, so instead use .addEventListener('click',function(){...}).

Generating HTML

You're doing a great job at using the proper DOM methods to build up your html, however sometimes code becomes more readable by building up a html string which you apply with .innterHTML. Or alternatively using a templating library might be an even better idea, though you would have to find one which fits your taste (just look around on google, I used to use pure.js for example).

\$\endgroup\$
3
  • \$\begingroup\$ I did not use CSS to show their knowledge of javascript! probably it was wrong) about advice, thank you very much, you made ​​a contribution to the development of my future;) \$\endgroup\$
    – sparcmen
    Mar 6, 2014 at 23:19
  • \$\begingroup\$ @sparcmen If you want to thank the reviewer, up-vote his question :) \$\endgroup\$
    – syb0rg
    Mar 7, 2014 at 0:09
  • \$\begingroup\$ You seem like a re-activated user, you should come join us in the chat room sometime :) \$\endgroup\$
    – syb0rg
    Mar 7, 2014 at 1:00
3
\$\begingroup\$

From a once over:

  • This has the parameters split over multiple lines, that is to be avoided, you could do this:

    window.onload = function () {

  var rowCount = 20,
      columnCount = 6,
      refreshRate = 2000,
      grid = new Grid( rowCount, columnCount, refreshRate );
}
  • As David Mulder mentioned, your code could be a lot shorter if you moved more into CSS. In fact once you fixed that, I would re-submit this code. It would be interesting.
  • stopUpdateGrid should be part of grid, unless you meant all instances of Grid to stop in document.onkeyup which should be commented in that case.
  • Same goes for interval
  • You are never using grid in var grid = new Grid(rows, columns, updInterval); so you might as well just do new Grid(rows, columns, updInterval);
  • Other than that your code is very clean, JsHint.com could find close to nothing, it is readable, follows lowerCamelCasing, variables are well named, fun to review.
\$\endgroup\$
1
  • \$\begingroup\$ I will repurpose the code using CSS and acknowledge all the advice and comments) \$\endgroup\$
    – sparcmen
    Mar 7, 2014 at 5:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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