Updating Grid on Webpage

Question 1

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);
 };
 }
}

Question 2

Please leave the formatting alone. We are trying to make the question more readable for people who want to answer can understand the question.

Question 3

What are you going to do if the user resizes their browser

Question 4

I do not know why, but I never thought of that! then probably block "container" need to make 100%!

Question 5

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

Question 6

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

Question 7

@sparcmen If you want to thank the reviewer, up-vote his question :)

Question 8

You seem like a re-activated user, you should come join us in the chat room sometime :)

Question 9

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.

Question 10

I will repurpose the code using CSS and acknowledge all the advice and comments)

David Mulder David Mulder 4983 silver badges14 bronze badges · Answer 1 · 2014-03-06 23:11:03Z

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

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;)
@sparcmen If you want to thank the reviewer, up-vote his question :)
You seem like a re-activated user, you should come join us in the chat room sometime :)

konijn konijn 34.2k5 gold badges70 silver badges267 bronze badges · Answer 2 · 2014-03-07 01:56:00Z

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.

I will repurpose the code using CSS and acknowledge all the advice and comments)

Stack Exchange Network

Updating Grid on Webpage

2 Answers 2

Seperation of logic and presentation

Always use `addEventListener`

Generating HTML

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Hot Network Questions

Updating Grid on Webpage

2 Answers 2

Seperation of logic and presentation

Always use addEventListener

Generating HTML

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Related

Hot Network Questions

Always use `addEventListener`