geekhack
geekhack Community => Off Topic => Topic started by: njbair on Sun, 06 September 2015, 09:13:48
-
I was talking to a 12-year-old protege of mine who is really interested in web programming, but he can't think of anything to program. I suggested he do Conway's Game of Life, since it's got a simple set of rules and serves as a good introduction to many programming concepts.
Then it occurred to me that I had never written my own version of Life from scratch. So I decided to fix that. I wrote this yesterday afternoon in a few hours' time.
http://life.lab.nickbair.net/
It's still rough around the edges--no code optimization has been done, and I want to make a bunch of the options user-configurable, such as grid size, cycle delay, and maybe some common patterns as presets. But for now it's just Life.
Any programming nerds, I'd love your opinions, particularly on the source code structure.
-
for ... in loops are slightly naughty but if you control all the code on the page (no ads, frameworks, plugins etc!) then it'll be fine. Maybe slightly slower than a good old fashioned for loop, but I'm not sure about that.
Your init method is huge! I'd prefer to see it broken up into smaller well-named task methods just for the sake of readability.
Otherwise, very nice!
-
for ... in loops are slightly naughty but if you control all the code on the page (no ads, frameworks, plugins etc!) then it'll be fine. Maybe slightly slower than a good old fashioned for loop, but I'm not sure about that.
Your init method is huge! I'd prefer to see it broken up into smaller well-named task methods just for the sake of readability.
Otherwise, very nice!
Yeah, for...in loops will iterate through all object properties including any prototype properties. I don't know why I was using them there instead of a regular for loop.
After reading your comments I decided to run my JS through jshint. I was surprised it didn't yell at me for the for...in loops. But it did identify two anonymous functions within a loop in my init method, so I took the hint (no pun intended) and refactored my init method, offloading each operation into its own method. Having descriptive method names for each of those blocks of code makes the whole thing much more self-documenting. Plus, now my init method is tiny:
init: function() {
var x,y;
Life.board = document.getElementById(Life.config.boardElementId);
Life.grid = Life.createGrid();
Life.board.appendChild(Life.grid.element);
for (y=0; y<Life.grid.rows.length; y++) {
Life.grid.rows[y] = Life.createRow(y);
Life.grid.element.appendChild(Life.grid.rows[y].element);
for (x=0; x<Life.grid.rows[y].columns.length; x++) {
Life.grid.rows[y].columns[x] = Life.createCell(x,y);
Life.grid.rows[y].element.appendChild(Life.grid.rows[y].columns[x].element);
}
}
},
So thanks for the feedback. The code is much cleaner now.
-
I dunno about the code (haven't learned enough JavaScript to consider myself in a position to comment), but a "clear all" button would be nice :)
-
I dunno about the code (haven't learned enough JavaScript to consider myself in a position to comment), but a "clear all" button would be nice :)
Good idea. I added one but labeled it "reinitialize" because when I eventually add configurable width/height settings, I'll use that button to redraw the grid.
-
quick architectural review, I don't actually know wth Life is, so I'm not going into the code. Everything said, besides not enough comments, it's better than most grads by a significant margin.
1. Radix is set to 0
"Life.config.grid.width = parseInt(Life.controls.widthField.value, 0);"
this is an example of radix being set to 0 when calling parseInt(). There is no benefit to this because it's equivalent to not setting a radix at all.
More usefully, it should be set to 10 because most people will assume something like width will be in base10.
ref:
"parseInt(string, radix);"
"If radix is undefined or 0 (or absent)"
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt
2. Model is not explicitly attached to window object
"var Life = {...}" is the object/model for the application. It is declared at the root level of the script file, meaning this will attach the object implicitly to the window object.
A better solution would be to explicitly attach it to the window object - like this:
"window.Life = {...}"
Further improving the code, you should create an IIFE and import a 'namespace':
"
;(function (model) {
model = {...}; //'model' is equivalent to 'life'
})(window.Life = window.Life || {});
"
3. 'Life' is a data object, not a class
Capitalized names are reserved for classes according to every standard naming convention
4. Magic strings are frequently used
Examples include:
"hidden"
"gameContainer"
'static' strings, such as "hidden", should be made into a static string
'initialization' strings, such as 'gameContainer', should be injected into the model and saved into a string variable that is used instead of the literal
5. Driver is included with the core logic/model
The 'load' event callback serves as a driver in this codebase. It should be externalized into a separate JS file or made inline in the HTML file. This will force an API to be made with the result being enhanced testability (hopefully).
There are many more details about why your driver should be separate, but it's more detail than I care to dump out at this point.
6. setInterval is used
I'm not going to go into why this is bad. It really should only be used after careful planning and analysis. There ARE situations where it is the best or only option; but those are far rarer than instances where setInterval is used.
EDIT:
also no one tell me that JS doesn't have classes. if you know what you're talking about, then you know what I mean when i say classes.
also JS2015
-
quick architectural review, I don't actually know wth Life is, so I'm not going into the code. Everything said, besides not enough comments, it's better than most grads by a significant margin.
1. Radix is set to 0
"Life.config.grid.width = parseInt(Life.controls.widthField.value, 0);"
this is an example of radix being set to 0 when calling parseInt(). There is no benefit to this because it's equivalent to not setting a radix at all.
More usefully, it should be set to 10 because most people will assume something like width will be in base10.
ref:
"parseInt(string, radix);"
"If radix is undefined or 0 (or absent)"
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt
2. Model is not explicitly attached to window object
"var Life = {...}" is the object/model for the application. It is declared at the root level of the script file, meaning this will attach the object implicitly to the window object.
A better solution would be to explicitly attach it to the window object - like this:
"window.Life = {...}"
Further improving the code, you should create an IIFE and import a 'namespace':
"
;(function (model) {
model = {...}; //'model' is equivalent to 'life'
})(window.Life = window.Life || {});
"
3. 'Life' is a data object, not a class
Capitalized names are reserved for classes according to every standard naming convention
4. Magic strings are frequently used
Examples include:
"hidden"
"gameContainer"
'static' strings, such as "hidden", should be made into a static string
'initialization' strings, such as 'gameContainer', should be injected into the model and saved into a string variable that is used instead of the literal
5. Driver is included with the core logic/model
The 'load' event callback serves as a driver in this codebase. It should be externalized into a separate JS file or made inline in the HTML file. This will force an API to be made with the result being enhanced testability (hopefully).
There are many more details about why your driver should be separate, but it's more detail than I care to dump out at this point.
6. setInterval is used
I'm not going to go into why this is bad. It really should only be used after careful planning and analysis. There ARE situations where it is the best or only option; but those are far rarer than instances where setInterval is used.
EDIT:
also no one tell me that JS doesn't have classes. if you know what you're talking about, then you know what I mean when i say classes.
also JS2015
Wow, I feel like I've got a lot to learn from these points. Rest assured I'll be going over these at some point to learn what I can from them and apply the changes. Thanks!