HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Updating Grid on Webpage

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
webpageupdatinggrid

Problem

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.


    window.onload = function () {

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

    }


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 = 1

Solution

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).

Context

StackExchange Code Review Q#43655, answer score: 5

Revisions (0)

No revisions yet.