5
\$\begingroup\$

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();
\$\endgroup\$
  • 2
    \$\begingroup\$ Hello! Please don't make changes to the original post once it has been reviewed, as that invalidates the current answers. Please see our meta side on performing iterative reviews for more information! \$\endgroup\$ – syb0rg Jul 13 '16 at 18:40
5
\$\begingroup\$

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 ;)
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot ! I modified stuff according to your advices : data structures, better named variables, no magic numbers, removed prefixes. That leaves me with 2 things I'm not sure : self executing query, you mean I have to ditch the pattern then ? Considering the second question : to be honest that's what I wanted to do from start, using a MVC pattern. It makes much sense. But I really have no idea how to do that.. should I use 3 different module patterns ? that call each other ? How would the "flow" of the game works ? Basically my big game() function would be all in controller ? \$\endgroup\$ – Antonin Cezard Jul 13 '16 at 18:39
  • \$\begingroup\$ I will write my andapproach with MVC and post it as a coderview question, I will place the link here. Also, feel free to mark my answer as the accepted one ;) \$\endgroup\$ – konijn Jul 13 '16 at 18:41
  • \$\begingroup\$ @topleft : codereview.stackexchange.com/questions/134807/… \$\endgroup\$ – konijn Jul 13 '16 at 21:27

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.