11
\$\begingroup\$

Previous version:

Updating Grid on Webpage

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.

Corrected:

  1. object passed to the constructor with parameters
  2. variables moved inside the constructor grid
  3. function createGrid as a method of object inside the constructor
  4. use style CSS
  5. added function that keeps track of dynamic resizing the browser window

HTML:

<!DOCTYPE HTML>

<html>
<head>
<meta charset="utf-8">
<title></title>
<link href="css/gridStyle.css" rel="stylesheet" type="text/css">
<script type="text/javascript" src="js/grid.js"></script>
<script type="text/javascript">

    window.onload = function () {

        var options = {
            numRows : 20,
            numColumns : 6,
            updateInterval : 2000
        }

        var grid = new Grid (options);         

    }

</script>

</head>
<body>
</body>
</html>

JS:

function Grid(gridOptions) {

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

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

    var interval=0;
    var checkInterval;
    var stopUpdateGrid = false;

    this.createGrid = function(){
        var container = document.createElement("div");
        document.body.appendChild(container);
        container.className="container";
        for (var i = 1; i <= this.numRows * this.numColumns; i++) {
            var cell = document.createElement("div");
            container.appendChild(cell);
            cell.className="cell";
            cell.style.width = (clientWidth - this.numRows - 1) / this.numRows + "px";
            cell.style.height = (clientHeight - this.numColumns - 1) / this.numColumns + "px";
            var n = randomColors();
            cell.style.backgroundColor = "#" + n;
        }

        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(updateInterval) {
            if (interval) {
                clearTimeout(interval);
            }
            interval = setTimeout(function() {
                if (stopUpdateGrid === false) {
                    var container = document.getElementsByClassName("container")[0];
                    document.body.removeChild(container);
                    var grid = new Grid(gridOptions);
                }
            }, updateInterval);
        })(this.updateInterval);

        document.addEventListener("keyup", function(e) {
            e = e || window.event;
            if (e.keyCode === 13) {
                stopUpdateGrid = true;
                changeGridOptions();
            }
        }, false);

        (function createNotification() {
            var container = document.getElementsByClassName("container")[0];
            var notificationContainer = document.createElement("div");
            container.appendChild(notificationContainer);
            notificationContainer.className = "notification";
            notificationContainer.innerHTML = "Press Enter to show menu change!!!";
        })();

        function changeGridOptions() {
            var container = document.getElementsByClassName("container")[0];
            var changeOptionsForm = document.createElement("form");
            container.appendChild(changeOptionsForm);
            changeOptionsForm.className = "changeOptionsForm";
            changeOptionsForm.style.left = clientWidth - 220 + "px";
            changeOptionsForm.style.top = "15px";

            var changeRowsText = document.createElement("div");
            changeOptionsForm.appendChild(changeRowsText);
            changeRowsText.className = "changeParametersTitle";
            changeRowsText.innerHTML = "Rows:";

            var changeRows = document.createElement("input");
            changeOptionsForm.appendChild(changeRows);
            changeRows.type = "text";
            changeRows.className = "changeParametersValue";

            var changeColumnsText = document.createElement("div");
            changeOptionsForm.appendChild(changeColumnsText);
            changeColumnsText.className = "changeParametersTitle";
            changeColumnsText.innerHTML = "Columns:";

            var changeColumns = document.createElement("input");
            changeOptionsForm.appendChild(changeColumns);
            changeColumns.type = "text";
            changeColumns.className = "changeParametersValue";

            var changeDelayUpdateGridText = document.createElement("div");
            changeOptionsForm.appendChild(changeDelayUpdateGridText);
            changeDelayUpdateGridText.className = "changeParametersTitle";
            changeDelayUpdateGridText.innerHTML = "Delay Update:";

            var changeDelayUpdateGrid = document.createElement("input");
            changeOptionsForm.appendChild(changeDelayUpdateGrid);
            changeDelayUpdateGrid.type = "text";
            changeDelayUpdateGrid.className = "changeParametersValue";

            var changeButton = document.createElement("input");
            changeOptionsForm.appendChild(changeButton);
            changeButton.type = "button";
            changeButton.value = "change";
            changeButton.className = "changeButton";

            changeButton.addEventListener("click", function() {
                gridOptions.numRows = changeRows.value || gridOptions.numRows;
                gridOptions.numColumns = changeColumns.value || gridOptions.numColumns;
                gridOptions.updateInterval = changeDelayUpdateGrid.value || gridOptions.updateInterval;
                changeOptionsForm.style.display = "none";
                stopUpdateGrid = false;
                document.body.removeChild(container);
                var grid = new Grid(gridOptions);
            }, false);

        }

        (function checkSizeWindowBrowser() {
            document.body.onresize = function() {
                var container = document.getElementsByClassName("container")[0];
                document.body.removeChild(container);
                clearTimeout(interval);
                var grid = new Grid(gridOptions);
            };
        })();


    };

    this.createGrid();

}

CSS:

html, body {
    width: 100%;
    height: 100%;
    margin: 0;
}

    .container {
        width: 100%;
        height: 100%;
    }

            .cell {
                float: left;
                margin: -1px 0 0 -1px;
                border: 1px solid #000;
            }

            .notification {
                width: 500px;
                height: 50px;
                color: #fff;
                font-size: 30px;
                position: absolute;
            }

            .changeOptionsForm {
                width: 200px;
                height: 200px;
                border: 2px solid #fff;
                background-color: #808080;
                position: absolute;
            }

                    .changeParametersTitle {
                        width: 50px;
                        height: 20px;
                        margin: 10px;
                        float: left;
                    }

                    .changeParametersValue {
                        width: 100px;
                        height: 20px;
                        margin: 10px;
                        border: 2px solid #000;
                    }

                    .changeButton {
                        width: 100px;
                        height: 25px;
                        margin: 20px 50px;
                        float: left;
                    }
\$\endgroup\$
  • \$\begingroup\$ Indent that CSS code properly! The JavaScript code looks so much better now, I will definitely get back to this. \$\endgroup\$ – konijn Mar 7 '14 at 23:11
5
\$\begingroup\$

I would not hard code 'container', instead make it an attribute of gridOptions :

var options = {   
   ....
   containerClass = 'container',
   ....
}

I would consider extending the options to include an id or selector of a parent (an anchor) where to add the grid (if null then use document.body as you do now).You could also add to the options the ID of the grid being created, then you would be potentially be able to handle multiple grids in the same document).

Every time the window is resized the grid is recreated, meaning that the keyup listener is attached to the document multiple times, and similarly changeGridOptions() is called every time the user presses the enter key, resulting in multiple calls to changeButton.addEventListener().

All these listeners should be removed (see https://stackoverflow.com/questions/14040386/should-i-remove-event-handlers-from-element-before-removechild) when the grid is cleared , ie when a new grid is created, by calling window.removeEventListener (See https://stackoverflow.com/questions/2991382/how-do-i-add-and-remove-an-event-listener-using-a-function-with-parameters)

There are 2 things that should happen when clearing the grid:

  1. the timeout should be cleared (only done in onresize())
  2. the container DOM element must be removed (implemented in 2 different places: the timer and on resize)

This duplication and inconsistency can be avoided by moving this code to the constructor of the grid: check if the container is already created (by class or by id if you provide one as I suggest above) and first clear the interval if it is set, then remove any registered listeners, then remove the DOM element, and then instantiate the Grid.

\$\endgroup\$
6
\$\begingroup\$

Overall it looks pretty clean,

  1. I'd like to see some comments on the functions so I know what they do without having to read the code, you could use JSDoc for this.
  2. I'd recommend explicitly using Design By Contract or Defensive Programming so the users of the code know what to expect. Which one of these you choose will determine what you put in the comments.
  3. Get the tabbing fixed on the CSS file.
  4. Use constants where you have code like this: e.keyCode === 13 to make the meaning clearer.
  5. Ideally pull the text that is shown to the user out into a language file that defines appropriate variables, so that if you want to port to another language, you only have to send that to the translator, not the code.
\$\endgroup\$
5
+50
\$\begingroup\$

Chris Thomson's answer makes some very good points about the overall quality, and Leopoldo Salvo's answer points out several hidden issues with the timer(s) and event handling. My comments will be more about alternative approaches to various parts. Not much code, more of a reinterpretation of the stated task.

The first thing I notice is that each update creates an entirely new grid. Even window resizing. It's a simple solution, but a pretty brute-force one.

A simpler solution is to let the browser do a lot of the work:
Make the container fill the window (e.g. absolute position and a 0px offset from all four sides with width/height set to auto).

Then style each cell with float: left, and all you have to worry about is their width and height. They'll automatically flow to fill the container, without you ever having to calculate their position.

And use percentages for the cells' width and height. That way, you never have to worry resizing either. You'll simply have a flat list of DIVs that automagically arrange themselves as a grid and fill the window.

This also makes it much simpler to have just 1 Grid instance that you update, instead of constantly destroying the entire thing, and building a new one. Here's a small example that does all that.

Now, another thing is that the task is unclear: Must the "context menu" be built entirely in JavaScript? The grid obviously must be, but the options aren't dynamic in the same way. I mean, if it's OK to use predefined CSS, and the page isn't supposed to do anything else, I'd absolutely create the options menu in the markup, because it's just easier, cleaner, and much more maintainable than using the DOM. All you'd have to do is hook up some event listeners, and bingo.

Yes, there'll be a lot more coupling between the markup and the javascript, but as mentioned it's already coupled to the CSS anyway, and the task, as defined, doesn't prohibit it. In terms of simply solving the task, adding the extra coupling just makes things much simpler. There are many ways to complicate things, but there's no reason to do so if it isn't required.

A completely separate issue is that "context menu" usually means a right-click menu - but building such a thing - and have it behave properly - is tricky, and would seem out of scope compared to the other parts of the task. Besides, it'd be an action menu; not editable fields.

Some other stuff I happened to notice:

  1. On the timed updates, it'd be better to simply loop though and update the colors of the existing DIVs, instead of removing everything and building a new grid. The number of elements or their positions haven't changed, so it's really only a question of setting the background color.

  2. Speaking of, you're working too hard to get a random color. Here's a nice one-liner you can use instead:

    '#' + Math.floor(Math.random()*0xFFFFFF).toString(16);
    
  3. Make the container absolutely positioned in CSS, and give it a 0px offset from all of the four sides. That'll make it fill the window (in modern browsers). Then style each cell with float: left, and all you have to worry about is their width and height - they'll automatically flow to fill the container.

  4. You're using a form for the "menu", but you're only handling the input when the button is clicked; not when the submit event of the form. The button doesn't submit the form either. As such, the form isn't really necessary - it could be any element. A form is of course semantically correct, but it should be used to your advantage.

  5. The way the form is being built also feels too complicated: Much more can be accomplished in CSS. The input and labels do not need classes, since more generic selectors (e.g. form input[type=text]) will be enough to style the correct elements.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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