patternjavascriptMinor
Updating Grid on Webpage
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.
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
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
Always use
In a simple stand alone case it doesn't matter, but at some point later you often end up regretting using things like
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
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
addEventListenerIn 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.