HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Rock-Paper-Scissors with the revealing module pattern

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scissorspaperthewithmodulerevealingrockpattern

Problem

In my ongoing effort in learning advanced JavaScript I tried to develop the traditional rock paper scissors game by using the famous revealing module pattern. I'm not comfortable yet with what has to be Private/Public. In my solution, I decided to have a single public function: Game. It uses a lot of private stuff and organizes the global game behavior. Everything else is private and not available to the caller. I'm not sure if it's a correct use of that pattern ?

Anyway I commented my code a lot I hope it will explain my intent correctly. I'm looking for any advices I can get, even if not directly related to the pattern I used.

```
var roshambo = (function() {

// We will add +1 for each human victory
var privateGameDB = 0;
// We will add +1 for each human or robot victory
var privateGameCount = 0;

// Get human input with validation, returns a number between 1 and 3
function privateGetInput() {
var choice = prompt('Rock: 1, Paper: 2, Scissors: 3');

// Input validation
while (parseInt(choice) = 4 || isNaN(parseInt(choice))) {
choice = prompt('Rock: 1, Paper: 2, Scissors:3 (your choice vas not valid)');
}

return parseInt(choice);
}

// Generate robot's play, randomize a number between 1 and 3
// Also includes a display box alerting the robot's play
function privateGetRobotPlay() {
var play = Math.floor(Math.random() * 3) + 1;

switch (play) {
case 1:
alert('Robot played Rock');
break;
case 2:
alert('Robot played Paper');
break;
case 3:
alert('Robot played Scissors');
break;
}

return play;
}

// Check who wins the round, by comparing the two numbers.
// Returns 0 on draw, 1 on loss and 2 on win
function privateComparePlays(human, robot) {
var humanPlay = human;
var robotPlay = robot;

// Tie
if (robotPlay == humanPlay) {
return 0;
}

// Robot wins
if (robotPlay == 1 && humanPlay == 3 || robot

Solution

I like your question,

-
I think if you are going to export one function, and then immediately execute it, then you are better served with a self executing query.

-
You are writing a game, I would use a revealing pattern for the model, the controller, and the view.

  • View would export



  • An alert function



  • An input function



  • Controller would export a



  • init or start function



  • It would have a lot of internal functions calling view and model



  • Model would export a



  • Getter/setter for victories



  • comparePlays function



-
Always consider if you can replace control structures with data structures:

// Display a message depending on round result
function privateAlertRoundResults(results) {
  var resultsMessages = [ 'Draw !', 'You lose !','You win !'];
  alert( resultsMessages[ results ];
}


instead of

// Display a message depending on round result
function privateAlertRoundResults(results) {
  switch (results) {
    case 0:
      alert('Draw !');
      break;
    case 1:
      alert('You lose !');
      break;
    case 2:
      alert('You win !');
      break;
  }
}


For extra points, consider how you can convert this into a data structure:

if (robotPlay == 1 && humanPlay == 3 || robotPlay == 2 && humanPlay == 1 || robotPlay == 3 && humanPlay == 2) {


-
Consider wellnamed variables or constants instead of magic numbers, your code would be so much more readable if you had upfront

var DRAW = 0,
    LOSE = 1,
    WIN  = 2;


and then later

function privateUpdateDB(check) {
  switch (check) {
    case LOSE:
      privateGameCount++;
      break;
    case WIN:
      privateGameDB++;
      privateGameCount++;
      break;
  }
}


of course in this particular case, I would not have gone for a case but an if:

function privateUpdateDB(check) {
  if( check == LOSE ){
      privateGameCount++;
  } else if ( check == WIN ){ 
      privateGameDB++;
      privateGameCount++;
  }
  //We don't increase privateGameCount for Draws (?)
}


  • I like your adjusting prompt in case the user provides a bad number



  • On the whole, prefixing your private functions with private is not a great idea because it interrupts the reading flow. I would not prefix at all, but if you insist on prefixing I would prefix with _, most developers will know what you mean (and frown because they also don't like prefixing ;)

Code Snippets

// Display a message depending on round result
function privateAlertRoundResults(results) {
  var resultsMessages = [ 'Draw !', 'You lose !','You win !'];
  alert( resultsMessages[ results ];
}
// Display a message depending on round result
function privateAlertRoundResults(results) {
  switch (results) {
    case 0:
      alert('Draw !');
      break;
    case 1:
      alert('You lose !');
      break;
    case 2:
      alert('You win !');
      break;
  }
}
if (robotPlay == 1 && humanPlay == 3 || robotPlay == 2 && humanPlay == 1 || robotPlay == 3 && humanPlay == 2) {
var DRAW = 0,
    LOSE = 1,
    WIN  = 2;
function privateUpdateDB(check) {
  switch (check) {
    case LOSE:
      privateGameCount++;
      break;
    case WIN:
      privateGameDB++;
      privateGameCount++;
      break;
  }
}

Context

StackExchange Code Review Q#134719, answer score: 5

Revisions (0)

No revisions yet.