6
\$\begingroup\$

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 = [
              "<img src=\"http://103.imagebam.com/download/CTHPzR99HvxiHwLlnY7ORA/27274/272731499/one.png\" data-brand-name=\"coca cola\">",
              "<img src=\"http://107.imagebam.com/download/jlBQih1wkuYKF45fgh7fJg/27274/272731502/two.png\" data-brand-name=\"durex\">",
              "<img src=\"http://101.imagebam.com/download/dz31UIrHKVzQy6j9sovh3w/27274/272731500/three.png\" data-brand-name=\"johnson and johnson\">",
              "<img src=\"http://107.imagebam.com/download/Q4d8eWgNRSO8RUPaPDLU4Q/27274/272731498/four.png\" data-brand-name=\"nike\">"
            ];

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 < ($brandLength); i++ ) {
//     if ( i === 0 ) {
//       $theinputs.append('<input type=\"text\" placeholder=\"tap me\">');
//     }
//     else {
//       $theinputs.append('<input type=\"text\">');
//     }
//   }
// }


// Find the number of inputs needed with the brand name, located in the datta attr
function numOfInputs() {
  var $brandName = $('.the-image img').data('brand-name');
  var brandLength = $brandName.length;
  var reg = new RegExp('\\s');

  // var space = $brandName.indexOf(' ');
  // var $numOfSpaces = $brandName.split(' ').length - 1;
  // var len = $brandLength - $numOfSpaces;

  for ( var i = 0; i < brandLength; i++ ) {
    // if ( i === space ) {
    //   $theinputs.append('<br>');
    // }

    if ( $brandName[i].match(reg) ) {
      $theinputs.append('<br>');
    }

    else {
      $theinputs.append('<input type=\"text\">');
    }
  }
}




// Find the brand name from the data attr to match it to the users answer
function findBrandName() {
  $brandName = $('.the-image img').data('brand-name');
  return $brandName;
}



function addPointToScore() {
  score += 1;
  $scoreDisplay.html(score);
}



function oneLessAttempt() {
  attempts -= 1;
  $attemptsDisplay.html(attempts);
  $attemptsDisplay.toggleClass('animated').toggleClass('pulse');
}



function oneLessSkip() {
  numOfSkips -= 1;
  $skipsLefDisplay.html(numOfSkips);
}




// Validation: make the inputs one character only, focus on the next input after character is entered, and allow backspace, delete, tab && shift...
function validate() {
  $('input').keyup(function(event) {
    var keyCode = event.keyCode || event.which;
    var val = $(this).val();

    if ( $(this).next().is('br') ) {
      $(this).next().next().focus();
     }

     else if(val.length === 1) {
      $(this).next().focus();
      }

     else if (keyCode === 8 || keyCode === 9 || keyCode === 16 || keyCode === 46 || (keyCode === 16 && keyCode === 9)) {
      return false;
     }

     else {
      val = val.substring(0, val.length - 1);
      $(this).val(val);
      $(this).next().focus()
    }
  });
}




function getBrandIndex() {
  $brandIndexDisplay.html(imageCount + 1 + "/" + images.length);
}




function loadNextBrand() {
  userAnswer = ''; // Reset user answer
  imageCount += 1; // Add one to the image array
  attempts = 3;
  $attemptsDisplay.html(attempts)
  newBrand(imageCount); // load in the new brand image
  $theinputs.empty(); // clear the inputs
  findBrandName(); // Fetch the brand name from data attr
  numOfInputs(); // Set the number of inputs
  validate(); // Rerun the validation func
}






$(function() {

// Initial load, set up the page
  $theimage.html(images[imageCount]);
  numOfInputs();
  $scoreDisplay.html(score);
  $attemptsDisplay.html(attempts);
  $skipsLefDisplay.html(numOfSkips);
  getBrandIndex();
  validate();



// Submit answer
  $button.click(function() {
    $brandName = $('.the-image img').data('brand-name').replace(/\s+/g,"");

    $('.the-inputs input').each(function() {
      userAnswer += $(this).val();
    });

    userAnswer = $.trim(userAnswer).toLowerCase().replace(/\s+/g,"");

    if ( userAnswer === $brandName ) {
      addPointToScore();
      $('.modal').show().addClass('fade');

        if (score === 1) {
          $('.modal-content').html('<h2>Great work!</h2><p>You have <span>' + score + ' point</span></p><button class=\"next\">On & Up?</button>');
        }
        else {
          $('.modal-content').html('<h2>Great work!</h2><p>You have <span>' + score + ' points</span></p><button class=\"next\">On & Up?</button>');
        }

      $('.next').click(function() {
        $('.modal').hide();
      });

      loadNextBrand();
      getBrandIndex();
    }

    else if (images.length - 1 === imageCount) {
      $('.modal').show().addClass('fade');
      $('.modal-content').html('<h2>Bravo, that\'s the end of the game!</h2><p>You scored <span>' + score + ' points</span></p><p>Don\'t forget to screenshot your score!');
    }

    else {
      oneLessAttempt();

      $('.modal').show().addClass('fade');

        if (attempts === 1) {
          $('.modal-content').html('<h2>Ah shoot! Your guess was incorrect...</h2><p>You have <span>' + attempts + ' attempt</span> remaining</p><button class=\"cont\">Try Again</button>');
        }

        else {
          $('.modal-content').html('<h2>Ah shoot! Your guess was incorrect...</h2><p>You have <span>' + attempts + ' attempts</span> remaining</p><button class=\"cont\">Try Again</button>');
        }

      $('.cont').click(function() {
        $('.modal').hide();
      });

      if (attempts === 0) {
        $('.modal').show().addClass('game-over');
        $('.modal-content').html('<h2>Bah, it\'s game over!</h2><button class=\"restart\">Start Again?</button>');
        $('.restart').click(function() {
          location.reload();
        });
      }

      $('.the-inputs input').each(function() {
        $(this).val('');
      });

      userAnswer = '';
    }
  });



// Click skip
  $skip.click(function() {
    $('.modal').show().addClass('fade');
    $('.modal-content').html('<h2>Are you sure you want to skip? You will not get any points</h2><button class=\"yes\">Yes</button><button class=\"no\">No</button>');

    $('.yes').click(function() {
      $('.modal').hide();
      loadNextBrand();
      getBrandIndex();
      oneLessSkip();

      if (numOfSkips === 0) {
        $('.skip').remove();
      }
    });

    $('.no').click(function() {
      $('.modal').hide();
    });

    if (images.length - 1 === imageCount) {
      $('.modal').show().addClass('fade');
      $('.modal-content').html('<h2>Bravo, that\'s the end of the game!</h2><p>You scored <span>' + score + ' points</span></p><p>Don\'t forget to screenshot your score!</p><button class=\"restart\">Start Again?</button>');

        $('.restart').click(function() {
          location.reload();
        });
    }

    else if (numOfSkips === 0) {
      console.log('out of skips');
    }
  });

});
\$\endgroup\$
  • \$\begingroup\$ Bug report: I notice if I start typing very fast that it will not enter my letters correctly. :) \$\endgroup\$ – JasonG Aug 28 '13 at 13:46
  • \$\begingroup\$ Yes, I know. But I don't know why! Will keep battling... \$\endgroup\$ – James Aug 28 '13 at 14:35
  • \$\begingroup\$ @mnhg RE: put on hold If there was an easier way to paste in my JS, I would (like being able to wrap it in tags [code][/code] for example). However, the current 4 or 8 spaces does not work for my code. Half gets put in code blocks, half doesn't. Tell me if I'm missing something here? \$\endgroup\$ – James Aug 29 '13 at 8:22
8
\$\begingroup\$

While browsing the code with a first glance. 2 things come to mind:

  • 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 <br> to seperate the words. For this you have this strange if ( is('br') part in your code. Drop the <br> and put the words in different <div class="word"> 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:

Want to write really good code? Here are some fun articles:

good luck!

\$\endgroup\$
  • \$\begingroup\$ I added some links \$\endgroup\$ – Pinoniq Aug 28 '13 at 12:49
  • \$\begingroup\$ Thank you! This is exactly what I'm looking for, I really appreciate your time. \$\endgroup\$ – James Aug 28 '13 at 12:58
  • 1
    \$\begingroup\$ I like vanilla-js.com... Amusing \$\endgroup\$ – James Aug 28 '13 at 13:19
  • \$\begingroup\$ If you like the answer, accepting it as the correct one is always welcome :). And your welcome! thats why stackexchange exists \$\endgroup\$ – Pinoniq Aug 28 '13 at 13:19
  • \$\begingroup\$ Alrighty, sorted. \$\endgroup\$ – James Aug 28 '13 at 13:31
1
\$\begingroup\$

The keyboard handling needs work, I think.

When a new question is displayed, the first blank should automatically receive focus.

When typing a letter into a blank that already has a character, I would expect the previous character to be overwritten. However, else if(val.length === 1) { $(this).next().focus(); } just moves focus to the next blank.

When typing fast, some characters get lost. I'm not sure how to advise you to fix that problem.

To advance to the next blank, instead of $(this).next().next().focus(), you can do $(this).next('input').focus().

I would change the "Go!" button to an <input type="submit">, and put the UI inside a <form>. Not only would it make $(this).next('input').focus() work at the end, it would also enable the Enter key to work.

\$\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.