patternjavascriptMinor
"Guess the Brand" game
Viewed 0 times
gamethebrandguess
Problem
This is my first JavaScript/jQuery project after having been reading and learning for a good few weeks. I am proud I have made something that at least works. However, I'm sure that my code is inefficient, and possibly more complex that it needs to be. I would appreciate any feedback on my JavaScript. What other ways would I go about making this game?
As I started making it, I just thought up features and kind of "bolted" them on. So, with some more forethought, I could have been more organised.
The game is made for a mobile device, but for now it's browser only. I understand that the game isn't difficult (that's not what I'm aiming for at the moment), it's more the logic of the game, and the practice manipulating the DOM.
I want to use more objects in my code. I know how to create and use them, but I don't know how to practically use them.
The full game is available here.
```
var images = [
"",
"",
"",
""
];
var $theimage = $('.the-image');
var $theinputs = $('.the-inputs');
var $button = $('.go');
var $skip = $('.skip');
var $brandName;
var $scoreDisplay = $('.score span');
var $attemptsDisplay = $('.attempts span');
var $brandIndexDisplay = $('.brand-index');
var $skipsLefDisplay = $('.skips-left');
var score = 0;
var attempts = 3;
var userAnswer = '';
var imageCount = 0;
var numOfSkips = 3;
// Add a new image for a new go
function newBrand(i) {
$theimage.html(images[i]);
}
// // Find the number of inputs needed with the brand name, located in the datta attr This is per word (we switched to per letter....)
// var numOfInputs = function() {
// var $brandLength = $('.the-image img').data('brand-name').split(' ').length;
// // Loop through them all and append an input per word in the brand name
// for ( var i = 0; i ');
// }
// else {
// $theinputs.append('');
// }
// }
// }
// Find the number of inputs needed with the brand name, located in the datta attr
funct
As I started making it, I just thought up features and kind of "bolted" them on. So, with some more forethought, I could have been more organised.
The game is made for a mobile device, but for now it's browser only. I understand that the game isn't difficult (that's not what I'm aiming for at the moment), it's more the logic of the game, and the practice manipulating the DOM.
I want to use more objects in my code. I know how to create and use them, but I don't know how to practically use them.
The full game is available here.
```
var images = [
"",
"",
"",
""
];
var $theimage = $('.the-image');
var $theinputs = $('.the-inputs');
var $button = $('.go');
var $skip = $('.skip');
var $brandName;
var $scoreDisplay = $('.score span');
var $attemptsDisplay = $('.attempts span');
var $brandIndexDisplay = $('.brand-index');
var $skipsLefDisplay = $('.skips-left');
var score = 0;
var attempts = 3;
var userAnswer = '';
var imageCount = 0;
var numOfSkips = 3;
// Add a new image for a new go
function newBrand(i) {
$theimage.html(images[i]);
}
// // Find the number of inputs needed with the brand name, located in the datta attr This is per word (we switched to per letter....)
// var numOfInputs = function() {
// var $brandLength = $('.the-image img').data('brand-name').split(' ').length;
// // Loop through them all and append an input per word in the brand name
// for ( var i = 0; i ');
// }
// else {
// $theinputs.append('');
// }
// }
// }
// Find the number of inputs needed with the brand name, located in the datta attr
funct
Solution
While browsing the code with a first glance. 2 things come to mind:
There is no encapsulation of the code, this isn't bad since it simply works. But changing the classnames in your html is un-doable since you then also need to change the business logic of your code. To put it in a more professional sentence: your code is Tightly coupled thus rendering it very hard to use in another project.
Next to being Tightly coupled, the entire code is written in the global scope. This doesn't play nice when including other libraries. A very good read on understanding scope and context in javascript: http://ryanmorr.com/understanding-scope-and-context-in-javascript/
A part from these jQuery-programmers-mistakes (no pun intended, but most people tend to solve every answer with jQuery without knowing how JS really works) there are some small remaks on the code:
You just need one element returned here, so use an ID instead of a class. A class '.the-image' is very general and could also be used somewhere else in the code.
These variables are available EVERYWHERE since they are defined in the global scope. In fact these should actually be encapsulated. (The Module pattern is an easy way to do this).
I noticed you are using
Then, why jQuery?
The only thing you are doing is some simple dom selection. Plain Vanilla can do this: https://gist.github.com/liamcurry/2597326 oh, and have a look here: http://www.doxdesk.com/img/updates/20091116-so-large.gif
Want to read more about JAvascript? here are some really cool links:
Want to write really good code? Here are some fun articles:
good luck!
- This code seems to have no Portability / re-usability
- why jQuery?
There is no encapsulation of the code, this isn't bad since it simply works. But changing the classnames in your html is un-doable since you then also need to change the business logic of your code. To put it in a more professional sentence: your code is Tightly coupled thus rendering it very hard to use in another project.
Next to being Tightly coupled, the entire code is written in the global scope. This doesn't play nice when including other libraries. A very good read on understanding scope and context in javascript: http://ryanmorr.com/understanding-scope-and-context-in-javascript/
A part from these jQuery-programmers-mistakes (no pun intended, but most people tend to solve every answer with jQuery without knowing how JS really works) there are some small remaks on the code:
var $theimage = $('.the-image');
var $theinputs = $('.the-inputs');
var $button = $('.go');
var $skip = $('.skip');
var $brandName;
var $scoreDisplay = $('.score span');
var $attemptsDisplay = $('.attempts span');
var $brandIndexDisplay = $('.brand-index');
var $skipsLefDisplay = $('.skips-left');You just need one element returned here, so use an ID instead of a class. A class '.the-image' is very general and could also be used somewhere else in the code.
var score = 0;
var attempts = 3;
var userAnswer = '';
var imageCount = 0;
var numOfSkips = 3;These variables are available EVERYWHERE since they are defined in the global scope. In fact these should actually be encapsulated. (The Module pattern is an easy way to do this).
I noticed you are using
to seperate the words. For this you have this strange if ( is('br') part in your code. Drop the
and put the words in different `` containers. More flexiblity for styling different words, ... (just my 5 cent)Then, why jQuery?
The only thing you are doing is some simple dom selection. Plain Vanilla can do this: https://gist.github.com/liamcurry/2597326 oh, and have a look here: http://www.doxdesk.com/img/updates/20091116-so-large.gif
Want to read more about JAvascript? here are some really cool links:
- http://vanilla-js.com/
- http://eloquentjavascript.net/contents.html (one of my favorites)
- http://davidwalsh.name/javascript-objects (very good articles about scope and patterns)
- http://oscargodson.com/posts/what-the-fuck-is-prototypal-inheritance.html
- http://addyosmani.com/resources/essentialjsdesignpatterns/book/
- http://blog.bittersweetryan.com/2013/06/a-look-into-how-parameters-are-passed.html
Want to write really good code? Here are some fun articles:
- http://www.slideshare.net/nzakas/extreme-javascript-compression-with-yui-compressor
- http://thc.org/root/phun/unmaintain.html
- http://javascript.crockford.com/popular.html
good luck!
Code Snippets
var $theimage = $('.the-image');
var $theinputs = $('.the-inputs');
var $button = $('.go');
var $skip = $('.skip');
var $brandName;
var $scoreDisplay = $('.score span');
var $attemptsDisplay = $('.attempts span');
var $brandIndexDisplay = $('.brand-index');
var $skipsLefDisplay = $('.skips-left');var score = 0;
var attempts = 3;
var userAnswer = '';
var imageCount = 0;
var numOfSkips = 3;Context
StackExchange Code Review Q#30360, answer score: 8
Revisions (0)
No revisions yet.