Rock-Paper-Scissors with the revealing module pattern

Question

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) <= 0 || 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 || robotPlay == 2 && humanPlay == 1 || robotPlay == 3 && humanPlay == 2) {
      return 1;
    }

    // Defaults to player's victory
    return 2;
  }

  // Update game database and game counter, do nothing if the round is a tie
  function privateUpdateDB(check) {
    switch (check) {
      case 1:
        privateGameCount++;
        break;
      case 2:
        privateGameDB++;
        privateGameCount++;
        break;
    }
  }

  // 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;
    }
  }

  // Display a message with total winrate from 0 to 100% (rounded)
  function privateAlertTotalResults() {
    var results = Math.floor((privateGameDB / privateGameCount) * 100);
    alert('You won ' + results + '% of total games !');
  }

  // Check if the player wants to play again
  function privatePlayAgain() {
    var choice = confirm('Play again ?');
    return choice;
  }

  // Manage the whole game
  function publicGame() {
    // First we check who wins the round
    var humanPlay = privateGetInput();
    var robotPlay = privateGetRobotPlay();
    var roundResults = privateComparePlays(humanPlay, robotPlay);

    // Then display result to the user
    privateAlertRoundResults(roundResults);

    // If it's a tie, we do nothing.
    // If it's not : update database, display total winrate
    if (roundResults != 0) {
      privateUpdateDB(roundResults)
      privateAlertTotalResults();
    }

    // We end by asking the user if he wants to play again
    // If he does, we use a recursion to start the game again
    if (privatePlayAgain() == true) {
      publicGame();
    }
  }

  // The only function available to the user is the whole game
  return {
    play: publicGame
  };

})();

// Init a game
roshambo.play();

Answer 1

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 ;)