Rock Paper Scissors game using OOP











up vote
3
down vote

favorite












I'm a complete beginner to Java and we have just started learning objects and classes in school. I'm trying to create a very simple text-based Rock Paper Scissors game. I do realise that my code is a mess, so I'm asking for some suggestions on how could I improve it and if I'm even in the right direction with OOP.



Main class:



public class Main {
public static void main(String args) {
new Game();
}
}


Player class



public abstract class Player {
private String name;
private String choice;

public Player() {}
public Player(String name) {this.name = name;}

public void setName(String newName) {
name = newName;
}

public String getName() {
return name;
}

public String getChoice() {
return choice;
}

public void setChoice(String newChoice) {
choice = newChoice;
}
public abstract void selectChoice();
}


User class



import java.util.Scanner;

public class User extends Player {

private Scanner input;

public User() {
input = new Scanner(System.in);
}

public void selectChoice() {
System.out.println("Enter your choice: R - Rock, P - Paper, S - Scissors");
setChoice(input.nextLine().toUpperCase());
}
}


Computer class



import java.util.Random;

public class Computer extends Player {

private Random rand;
private final int MAX_NUMBER = 3;

public Computer() {
setName("Computer");
rand = new Random();
}

public void selectChoice() {
int randomNumber = rand.nextInt(MAX_NUMBER);
switch(randomNumber) {
case 0:
setChoice("ROCK");
break;
case 1:
setChoice("PAPER");
break;
case 2:
setChoice("SCISSORS");
break;
}
}
}


Game class



import java.util.Scanner;

public class Game {
private User p;
private Computer com;
private int playerWins;
private int playerLoses;
private int ties;
private boolean isRunning = false;
private Scanner scan;

public Game() {
p = new User();
com = new Computer();
scan = new Scanner(System.in);
start();
}

private void start() {
isRunning = true;
System.out.println("Please, enter your name:");
p.setName(scan.nextLine());

while(isRunning) {
displayScore();
p.selectChoice();
com.selectChoice();
displayChoices();
displayWinner(decideWinner());
updateScore(decideWinner());
playAgain();
}
}

private void displayScore() {
System.out.println(p.getName());
System.out.println("----------");
System.out.println("Wins: " + playerWins);
System.out.println("Loses: " + playerLoses);
System.out.println("Ties: " + ties);
System.out.println("----------");
}

private int decideWinner() {
// 0 - User wins
// 1 - Computer wins
// 2 - tie

if(p.getChoice().equals("ROCK") && com.getChoice().equals("SCISSORS"))
return 0;
else if(p.getChoice().equals("PAPER") && com.getChoice().equals("ROCK"))
return 0;
else if(p.getChoice().equals("SCISSORS") && com.getChoice().equals("PAPER"))
return 0;
else if(com.getChoice().equals("ROCK") && p.getChoice().equals("SCISSORS"))
return 1;
else if(com.getChoice().equals("PAPER") && p.getChoice().equals("ROCK"))
return 1;
else if(com.getChoice().equals("SCISSORS") && p.getChoice().equals("PAPER"))
return 1;
else
return 2;
}

private void displayChoices() {
System.out.println("User has selected: " + p.getChoice());
System.out.println("Computer has selected: " + com.getChoice());
}

private void displayWinner(int winner) {
switch(winner) {
case 0:
System.out.println("User has won!");
break;
case 1:
System.out.println("Computer has won!");
break;
case 2:
System.out.println("It is a tie!");
}
}

private void playAgain() {
String choice;
System.out.println("Do you want to play again? Enter Yes to play again.");
choice = scan.nextLine();
if(!(choice.toUpperCase().equals("YES") ))
isRunning = false;

}

private void updateScore(int winner) {
if(winner == 0)
playerWins++;
else if(winner == 1)
playerLoses++;
else if(winner == 2)
ties++;
}
}









share|improve this question









New contributor




Žiga Černič is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.




















  • What problem do you expect an OOP approach to solve for you? Design decisions should never be made in a vacuum.
    – jpmc26
    18 mins ago















up vote
3
down vote

favorite












I'm a complete beginner to Java and we have just started learning objects and classes in school. I'm trying to create a very simple text-based Rock Paper Scissors game. I do realise that my code is a mess, so I'm asking for some suggestions on how could I improve it and if I'm even in the right direction with OOP.



Main class:



public class Main {
public static void main(String args) {
new Game();
}
}


Player class



public abstract class Player {
private String name;
private String choice;

public Player() {}
public Player(String name) {this.name = name;}

public void setName(String newName) {
name = newName;
}

public String getName() {
return name;
}

public String getChoice() {
return choice;
}

public void setChoice(String newChoice) {
choice = newChoice;
}
public abstract void selectChoice();
}


User class



import java.util.Scanner;

public class User extends Player {

private Scanner input;

public User() {
input = new Scanner(System.in);
}

public void selectChoice() {
System.out.println("Enter your choice: R - Rock, P - Paper, S - Scissors");
setChoice(input.nextLine().toUpperCase());
}
}


Computer class



import java.util.Random;

public class Computer extends Player {

private Random rand;
private final int MAX_NUMBER = 3;

public Computer() {
setName("Computer");
rand = new Random();
}

public void selectChoice() {
int randomNumber = rand.nextInt(MAX_NUMBER);
switch(randomNumber) {
case 0:
setChoice("ROCK");
break;
case 1:
setChoice("PAPER");
break;
case 2:
setChoice("SCISSORS");
break;
}
}
}


Game class



import java.util.Scanner;

public class Game {
private User p;
private Computer com;
private int playerWins;
private int playerLoses;
private int ties;
private boolean isRunning = false;
private Scanner scan;

public Game() {
p = new User();
com = new Computer();
scan = new Scanner(System.in);
start();
}

private void start() {
isRunning = true;
System.out.println("Please, enter your name:");
p.setName(scan.nextLine());

while(isRunning) {
displayScore();
p.selectChoice();
com.selectChoice();
displayChoices();
displayWinner(decideWinner());
updateScore(decideWinner());
playAgain();
}
}

private void displayScore() {
System.out.println(p.getName());
System.out.println("----------");
System.out.println("Wins: " + playerWins);
System.out.println("Loses: " + playerLoses);
System.out.println("Ties: " + ties);
System.out.println("----------");
}

private int decideWinner() {
// 0 - User wins
// 1 - Computer wins
// 2 - tie

if(p.getChoice().equals("ROCK") && com.getChoice().equals("SCISSORS"))
return 0;
else if(p.getChoice().equals("PAPER") && com.getChoice().equals("ROCK"))
return 0;
else if(p.getChoice().equals("SCISSORS") && com.getChoice().equals("PAPER"))
return 0;
else if(com.getChoice().equals("ROCK") && p.getChoice().equals("SCISSORS"))
return 1;
else if(com.getChoice().equals("PAPER") && p.getChoice().equals("ROCK"))
return 1;
else if(com.getChoice().equals("SCISSORS") && p.getChoice().equals("PAPER"))
return 1;
else
return 2;
}

private void displayChoices() {
System.out.println("User has selected: " + p.getChoice());
System.out.println("Computer has selected: " + com.getChoice());
}

private void displayWinner(int winner) {
switch(winner) {
case 0:
System.out.println("User has won!");
break;
case 1:
System.out.println("Computer has won!");
break;
case 2:
System.out.println("It is a tie!");
}
}

private void playAgain() {
String choice;
System.out.println("Do you want to play again? Enter Yes to play again.");
choice = scan.nextLine();
if(!(choice.toUpperCase().equals("YES") ))
isRunning = false;

}

private void updateScore(int winner) {
if(winner == 0)
playerWins++;
else if(winner == 1)
playerLoses++;
else if(winner == 2)
ties++;
}
}









share|improve this question









New contributor




Žiga Černič is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.




















  • What problem do you expect an OOP approach to solve for you? Design decisions should never be made in a vacuum.
    – jpmc26
    18 mins ago













up vote
3
down vote

favorite









up vote
3
down vote

favorite











I'm a complete beginner to Java and we have just started learning objects and classes in school. I'm trying to create a very simple text-based Rock Paper Scissors game. I do realise that my code is a mess, so I'm asking for some suggestions on how could I improve it and if I'm even in the right direction with OOP.



Main class:



public class Main {
public static void main(String args) {
new Game();
}
}


Player class



public abstract class Player {
private String name;
private String choice;

public Player() {}
public Player(String name) {this.name = name;}

public void setName(String newName) {
name = newName;
}

public String getName() {
return name;
}

public String getChoice() {
return choice;
}

public void setChoice(String newChoice) {
choice = newChoice;
}
public abstract void selectChoice();
}


User class



import java.util.Scanner;

public class User extends Player {

private Scanner input;

public User() {
input = new Scanner(System.in);
}

public void selectChoice() {
System.out.println("Enter your choice: R - Rock, P - Paper, S - Scissors");
setChoice(input.nextLine().toUpperCase());
}
}


Computer class



import java.util.Random;

public class Computer extends Player {

private Random rand;
private final int MAX_NUMBER = 3;

public Computer() {
setName("Computer");
rand = new Random();
}

public void selectChoice() {
int randomNumber = rand.nextInt(MAX_NUMBER);
switch(randomNumber) {
case 0:
setChoice("ROCK");
break;
case 1:
setChoice("PAPER");
break;
case 2:
setChoice("SCISSORS");
break;
}
}
}


Game class



import java.util.Scanner;

public class Game {
private User p;
private Computer com;
private int playerWins;
private int playerLoses;
private int ties;
private boolean isRunning = false;
private Scanner scan;

public Game() {
p = new User();
com = new Computer();
scan = new Scanner(System.in);
start();
}

private void start() {
isRunning = true;
System.out.println("Please, enter your name:");
p.setName(scan.nextLine());

while(isRunning) {
displayScore();
p.selectChoice();
com.selectChoice();
displayChoices();
displayWinner(decideWinner());
updateScore(decideWinner());
playAgain();
}
}

private void displayScore() {
System.out.println(p.getName());
System.out.println("----------");
System.out.println("Wins: " + playerWins);
System.out.println("Loses: " + playerLoses);
System.out.println("Ties: " + ties);
System.out.println("----------");
}

private int decideWinner() {
// 0 - User wins
// 1 - Computer wins
// 2 - tie

if(p.getChoice().equals("ROCK") && com.getChoice().equals("SCISSORS"))
return 0;
else if(p.getChoice().equals("PAPER") && com.getChoice().equals("ROCK"))
return 0;
else if(p.getChoice().equals("SCISSORS") && com.getChoice().equals("PAPER"))
return 0;
else if(com.getChoice().equals("ROCK") && p.getChoice().equals("SCISSORS"))
return 1;
else if(com.getChoice().equals("PAPER") && p.getChoice().equals("ROCK"))
return 1;
else if(com.getChoice().equals("SCISSORS") && p.getChoice().equals("PAPER"))
return 1;
else
return 2;
}

private void displayChoices() {
System.out.println("User has selected: " + p.getChoice());
System.out.println("Computer has selected: " + com.getChoice());
}

private void displayWinner(int winner) {
switch(winner) {
case 0:
System.out.println("User has won!");
break;
case 1:
System.out.println("Computer has won!");
break;
case 2:
System.out.println("It is a tie!");
}
}

private void playAgain() {
String choice;
System.out.println("Do you want to play again? Enter Yes to play again.");
choice = scan.nextLine();
if(!(choice.toUpperCase().equals("YES") ))
isRunning = false;

}

private void updateScore(int winner) {
if(winner == 0)
playerWins++;
else if(winner == 1)
playerLoses++;
else if(winner == 2)
ties++;
}
}









share|improve this question









New contributor




Žiga Černič is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











I'm a complete beginner to Java and we have just started learning objects and classes in school. I'm trying to create a very simple text-based Rock Paper Scissors game. I do realise that my code is a mess, so I'm asking for some suggestions on how could I improve it and if I'm even in the right direction with OOP.



Main class:



public class Main {
public static void main(String args) {
new Game();
}
}


Player class



public abstract class Player {
private String name;
private String choice;

public Player() {}
public Player(String name) {this.name = name;}

public void setName(String newName) {
name = newName;
}

public String getName() {
return name;
}

public String getChoice() {
return choice;
}

public void setChoice(String newChoice) {
choice = newChoice;
}
public abstract void selectChoice();
}


User class



import java.util.Scanner;

public class User extends Player {

private Scanner input;

public User() {
input = new Scanner(System.in);
}

public void selectChoice() {
System.out.println("Enter your choice: R - Rock, P - Paper, S - Scissors");
setChoice(input.nextLine().toUpperCase());
}
}


Computer class



import java.util.Random;

public class Computer extends Player {

private Random rand;
private final int MAX_NUMBER = 3;

public Computer() {
setName("Computer");
rand = new Random();
}

public void selectChoice() {
int randomNumber = rand.nextInt(MAX_NUMBER);
switch(randomNumber) {
case 0:
setChoice("ROCK");
break;
case 1:
setChoice("PAPER");
break;
case 2:
setChoice("SCISSORS");
break;
}
}
}


Game class



import java.util.Scanner;

public class Game {
private User p;
private Computer com;
private int playerWins;
private int playerLoses;
private int ties;
private boolean isRunning = false;
private Scanner scan;

public Game() {
p = new User();
com = new Computer();
scan = new Scanner(System.in);
start();
}

private void start() {
isRunning = true;
System.out.println("Please, enter your name:");
p.setName(scan.nextLine());

while(isRunning) {
displayScore();
p.selectChoice();
com.selectChoice();
displayChoices();
displayWinner(decideWinner());
updateScore(decideWinner());
playAgain();
}
}

private void displayScore() {
System.out.println(p.getName());
System.out.println("----------");
System.out.println("Wins: " + playerWins);
System.out.println("Loses: " + playerLoses);
System.out.println("Ties: " + ties);
System.out.println("----------");
}

private int decideWinner() {
// 0 - User wins
// 1 - Computer wins
// 2 - tie

if(p.getChoice().equals("ROCK") && com.getChoice().equals("SCISSORS"))
return 0;
else if(p.getChoice().equals("PAPER") && com.getChoice().equals("ROCK"))
return 0;
else if(p.getChoice().equals("SCISSORS") && com.getChoice().equals("PAPER"))
return 0;
else if(com.getChoice().equals("ROCK") && p.getChoice().equals("SCISSORS"))
return 1;
else if(com.getChoice().equals("PAPER") && p.getChoice().equals("ROCK"))
return 1;
else if(com.getChoice().equals("SCISSORS") && p.getChoice().equals("PAPER"))
return 1;
else
return 2;
}

private void displayChoices() {
System.out.println("User has selected: " + p.getChoice());
System.out.println("Computer has selected: " + com.getChoice());
}

private void displayWinner(int winner) {
switch(winner) {
case 0:
System.out.println("User has won!");
break;
case 1:
System.out.println("Computer has won!");
break;
case 2:
System.out.println("It is a tie!");
}
}

private void playAgain() {
String choice;
System.out.println("Do you want to play again? Enter Yes to play again.");
choice = scan.nextLine();
if(!(choice.toUpperCase().equals("YES") ))
isRunning = false;

}

private void updateScore(int winner) {
if(winner == 0)
playerWins++;
else if(winner == 1)
playerLoses++;
else if(winner == 2)
ties++;
}
}






java beginner rock-paper-scissors






share|improve this question









New contributor




Žiga Černič is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











share|improve this question









New contributor




Žiga Černič is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









share|improve this question




share|improve this question








edited 1 hour ago









200_success

127k15149412




127k15149412






New contributor




Žiga Černič is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









asked 3 hours ago









Žiga Černič

161




161




New contributor




Žiga Černič is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.





New contributor





Žiga Černič is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.






Žiga Černič is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.












  • What problem do you expect an OOP approach to solve for you? Design decisions should never be made in a vacuum.
    – jpmc26
    18 mins ago


















  • What problem do you expect an OOP approach to solve for you? Design decisions should never be made in a vacuum.
    – jpmc26
    18 mins ago
















What problem do you expect an OOP approach to solve for you? Design decisions should never be made in a vacuum.
– jpmc26
18 mins ago




What problem do you expect an OOP approach to solve for you? Design decisions should never be made in a vacuum.
– jpmc26
18 mins ago










2 Answers
2






active

oldest

votes

















up vote
1
down vote













This code really isn't a mess! While there are a few relatively minor points I can make, your code is generally easy to read and understand, and there's nothing horribly wrong about it.




  1. Watch out for user input! I can enter Z (or anything but rock/paper/scissors) and the result will always be a tie. You should tell the user when they enter something invalid and have them correct it.


  2. Both Computer and Player use setName to set their name - once. I would prefer requiring the use of the Player constructor with the name parameter since you could then mark name as final.


  3. Why is setChoice public? It is (and should) only used by selectChoice, so mark it protected.


  4. What happens when I want to play a game between two computer opponents (or two people)? Right now I can't. It would be nice if Game let me pass the two Player objects in that will be opponents.


  5. While I appreciate the MAX_NUMBER constant in the Computer class instead of just including a magic number, it's a better idea to store your choices in an array and then select a random element of the array.



  6. Passing around strings for choices isn't so bad when just dealing with Rock/Paper/Scissors, but it will quickly become unmanagable. Enum Types were introduced to fix this problem, and generally do a pretty good job of it! It wouldn't hurt to start using them now. Another good spot for an enumeration is in the decideWinner method.



    The Choice enum type could also provide a method winsAgainst(Choice other) that can be used to simplify decideWinner.



  7. For the most part, your names are great... until we get to Game and see p and com.


  8. It would be nice to use the user's name instead of User when displaying the winner and when displaying choices.



The following points may directly contradict what your teacher says, as they are more opinion based. Follow whatever your team's (or assignment's) guidelines say.





  1. Don't be afraid to include more than one line of code in main. If I wrote this program my main function would look something like this:



    Scanner scanner = new Scanner(System.in);
    System.out.println("Please enter your name:");

    Player user = new User(scanner.nextLine());
    Player computer = new Computer();
    Game game = new Game(user, computer);

    do {
    game.play();
    System.out.println("Play again? [Yes/No]");
    } while (scanner.nextLine().toUpperCase().equals("YES"));


  2. Use braces on all if statements that aren't one line long. I would be fine with if (winner == 0) playerWins++;, but if (winner == 0)n playerWins++; is much easier to mess up, especially if you don't have automatic indentation.


  3. Don't declare variables before initializing them, if possible. There's no reason to declare choice before printing out instructions in the playAgain method.







share|improve this answer




























    up vote
    1
    down vote













    After writing this review, I noticed there is still a few things that there were a some improvements that would require more-advanced-than-beginner levels of Java syntax. This answers focuses mostly on language agnostic improvements. There are some other things that can be added, but I really wanted to focus on the idea of a "Game Loop" before getting into too many details.



    Game loop



    You don't follow a typical game loop. Here is an article about creating a game loop. Fortunately, you don't need to worry about Frames Per Second (FPS) interpolation which is a large part of the article I linked. Nevertheless, it is imperative to look at this:



    bool game_is_running = true;

    while( game_is_running ) {
    update_game();
    display_game();
    }


    This logic should be in you main method. In other words, something like:



    public class Main {
    public static void main(String args) {
    Game game = new Game();
    game.start();
    while (game.isRunning()) {
    game.step();
    game.display();
    }
    }
    }


    Refactoring start



    Thus, it is imperative that we refactor this start method you have. First, make it public so I can call it as such. (There are actually a couple of other things related to starting, ending, and restarting games that you should probably do, but I think there is already enough in this CodeReview.)



    What is step?



    The step method mentioned above is actually located in your start. It is almost:



    public void step() {
    displayScore();
    p.selectChoice();
    com.selectChoice();
    displayChoices();
    displayWinner(decideWinner());
    updateScore(decideWinner());
    playAgain();
    }


    We need still change a couple things:




    1. This is more of stepAndDisplay not step.

    2. We will need to make some changes about the Player class (more on that later).


    Let's break step and display up.



    step should generate the data, and handle input. display should output the data. So:



    public void display() {
    displayOldScore();
    displayChoices();
    displayWinner();
    displayNewScore();
    playAgain();
    }


    And step could be:



    public void step() {
    oldScore = newScore;
    p.selectChoice();
    com.selectChoice();
    newScore += decideWinner();
    }


    Refactoring step even further.



    Instead of what I just wrote, the selectChoice() method is a bit awkward, I would just want to do:



    public void step() {
    oldScore = newScore;
    newScore += decideWinner(p.getChoice(), com.getChoice());
    }


    But, even this can worked on:




    Player class



    Now back to the idea of selectChoice: Just implement getChoice as what is currently selectChoice. For example the Computer method selectChoice becomes:



    public String getChoice() {
    int randomNumber = rand.nextInt(MAX_NUMBER);
    switch(randomNumber) {
    case 0:
    return "ROCK";
    case 1:
    return "PAPER";
    case 2:
    return "SCISSORS";
    }
    }


    Handling different player configurations



    Currently, you cannot have two human players play each other. I would imagine you want this option.



    Here are some changes that would need to be made in order for this to happen:



    // Each player should have type player, don't make it `User` and `Computer`.
    private Player p1;
    private Player p2;

    public Game() { // Default...
    p1 = new User();
    p2 = new Computer();
    scan = new Scanner(System.in);
    }

    public Game(Player firstPlayer, Player secondPlayer) {
    p1 = firstPlayer;
    p2 = secondPlayer;
    scan = new Scanner(System.in);
    }


    (There are further changes that I leave to you as an exercise: How do you change displayScore?)




    Computer class



    Your computer class is a particular kind of AI. Your Computer implements a "random selection" so I would change the class name to RandomComputer or something like that. Bottom line: You may want other computers in the future.





    1. However You need an end game function too as well as some other additions.






    share|improve this answer























      Your Answer





      StackExchange.ifUsing("editor", function () {
      return StackExchange.using("mathjaxEditing", function () {
      StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
      StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
      });
      });
      }, "mathjax-editing");

      StackExchange.ifUsing("editor", function () {
      StackExchange.using("externalEditor", function () {
      StackExchange.using("snippets", function () {
      StackExchange.snippets.init();
      });
      });
      }, "code-snippets");

      StackExchange.ready(function() {
      var channelOptions = {
      tags: "".split(" "),
      id: "196"
      };
      initTagRenderer("".split(" "), "".split(" "), channelOptions);

      StackExchange.using("externalEditor", function() {
      // Have to fire editor after snippets, if snippets enabled
      if (StackExchange.settings.snippets.snippetsEnabled) {
      StackExchange.using("snippets", function() {
      createEditor();
      });
      }
      else {
      createEditor();
      }
      });

      function createEditor() {
      StackExchange.prepareEditor({
      heartbeatType: 'answer',
      convertImagesToLinks: false,
      noModals: true,
      showLowRepImageUploadWarning: true,
      reputationToPostImages: null,
      bindNavPrevention: true,
      postfix: "",
      imageUploader: {
      brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
      contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
      allowUrls: true
      },
      onDemand: true,
      discardSelector: ".discard-answer"
      ,immediatelyShowMarkdownHelp:true
      });


      }
      });






      Žiga Černič is a new contributor. Be nice, and check out our Code of Conduct.










      draft saved

      draft discarded


















      StackExchange.ready(
      function () {
      StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209331%2frock-paper-scissors-game-using-oop%23new-answer', 'question_page');
      }
      );

      Post as a guest















      Required, but never shown

























      2 Answers
      2






      active

      oldest

      votes








      2 Answers
      2






      active

      oldest

      votes









      active

      oldest

      votes






      active

      oldest

      votes








      up vote
      1
      down vote













      This code really isn't a mess! While there are a few relatively minor points I can make, your code is generally easy to read and understand, and there's nothing horribly wrong about it.




      1. Watch out for user input! I can enter Z (or anything but rock/paper/scissors) and the result will always be a tie. You should tell the user when they enter something invalid and have them correct it.


      2. Both Computer and Player use setName to set their name - once. I would prefer requiring the use of the Player constructor with the name parameter since you could then mark name as final.


      3. Why is setChoice public? It is (and should) only used by selectChoice, so mark it protected.


      4. What happens when I want to play a game between two computer opponents (or two people)? Right now I can't. It would be nice if Game let me pass the two Player objects in that will be opponents.


      5. While I appreciate the MAX_NUMBER constant in the Computer class instead of just including a magic number, it's a better idea to store your choices in an array and then select a random element of the array.



      6. Passing around strings for choices isn't so bad when just dealing with Rock/Paper/Scissors, but it will quickly become unmanagable. Enum Types were introduced to fix this problem, and generally do a pretty good job of it! It wouldn't hurt to start using them now. Another good spot for an enumeration is in the decideWinner method.



        The Choice enum type could also provide a method winsAgainst(Choice other) that can be used to simplify decideWinner.



      7. For the most part, your names are great... until we get to Game and see p and com.


      8. It would be nice to use the user's name instead of User when displaying the winner and when displaying choices.



      The following points may directly contradict what your teacher says, as they are more opinion based. Follow whatever your team's (or assignment's) guidelines say.





      1. Don't be afraid to include more than one line of code in main. If I wrote this program my main function would look something like this:



        Scanner scanner = new Scanner(System.in);
        System.out.println("Please enter your name:");

        Player user = new User(scanner.nextLine());
        Player computer = new Computer();
        Game game = new Game(user, computer);

        do {
        game.play();
        System.out.println("Play again? [Yes/No]");
        } while (scanner.nextLine().toUpperCase().equals("YES"));


      2. Use braces on all if statements that aren't one line long. I would be fine with if (winner == 0) playerWins++;, but if (winner == 0)n playerWins++; is much easier to mess up, especially if you don't have automatic indentation.


      3. Don't declare variables before initializing them, if possible. There's no reason to declare choice before printing out instructions in the playAgain method.







      share|improve this answer

























        up vote
        1
        down vote













        This code really isn't a mess! While there are a few relatively minor points I can make, your code is generally easy to read and understand, and there's nothing horribly wrong about it.




        1. Watch out for user input! I can enter Z (or anything but rock/paper/scissors) and the result will always be a tie. You should tell the user when they enter something invalid and have them correct it.


        2. Both Computer and Player use setName to set their name - once. I would prefer requiring the use of the Player constructor with the name parameter since you could then mark name as final.


        3. Why is setChoice public? It is (and should) only used by selectChoice, so mark it protected.


        4. What happens when I want to play a game between two computer opponents (or two people)? Right now I can't. It would be nice if Game let me pass the two Player objects in that will be opponents.


        5. While I appreciate the MAX_NUMBER constant in the Computer class instead of just including a magic number, it's a better idea to store your choices in an array and then select a random element of the array.



        6. Passing around strings for choices isn't so bad when just dealing with Rock/Paper/Scissors, but it will quickly become unmanagable. Enum Types were introduced to fix this problem, and generally do a pretty good job of it! It wouldn't hurt to start using them now. Another good spot for an enumeration is in the decideWinner method.



          The Choice enum type could also provide a method winsAgainst(Choice other) that can be used to simplify decideWinner.



        7. For the most part, your names are great... until we get to Game and see p and com.


        8. It would be nice to use the user's name instead of User when displaying the winner and when displaying choices.



        The following points may directly contradict what your teacher says, as they are more opinion based. Follow whatever your team's (or assignment's) guidelines say.





        1. Don't be afraid to include more than one line of code in main. If I wrote this program my main function would look something like this:



          Scanner scanner = new Scanner(System.in);
          System.out.println("Please enter your name:");

          Player user = new User(scanner.nextLine());
          Player computer = new Computer();
          Game game = new Game(user, computer);

          do {
          game.play();
          System.out.println("Play again? [Yes/No]");
          } while (scanner.nextLine().toUpperCase().equals("YES"));


        2. Use braces on all if statements that aren't one line long. I would be fine with if (winner == 0) playerWins++;, but if (winner == 0)n playerWins++; is much easier to mess up, especially if you don't have automatic indentation.


        3. Don't declare variables before initializing them, if possible. There's no reason to declare choice before printing out instructions in the playAgain method.







        share|improve this answer























          up vote
          1
          down vote










          up vote
          1
          down vote









          This code really isn't a mess! While there are a few relatively minor points I can make, your code is generally easy to read and understand, and there's nothing horribly wrong about it.




          1. Watch out for user input! I can enter Z (or anything but rock/paper/scissors) and the result will always be a tie. You should tell the user when they enter something invalid and have them correct it.


          2. Both Computer and Player use setName to set their name - once. I would prefer requiring the use of the Player constructor with the name parameter since you could then mark name as final.


          3. Why is setChoice public? It is (and should) only used by selectChoice, so mark it protected.


          4. What happens when I want to play a game between two computer opponents (or two people)? Right now I can't. It would be nice if Game let me pass the two Player objects in that will be opponents.


          5. While I appreciate the MAX_NUMBER constant in the Computer class instead of just including a magic number, it's a better idea to store your choices in an array and then select a random element of the array.



          6. Passing around strings for choices isn't so bad when just dealing with Rock/Paper/Scissors, but it will quickly become unmanagable. Enum Types were introduced to fix this problem, and generally do a pretty good job of it! It wouldn't hurt to start using them now. Another good spot for an enumeration is in the decideWinner method.



            The Choice enum type could also provide a method winsAgainst(Choice other) that can be used to simplify decideWinner.



          7. For the most part, your names are great... until we get to Game and see p and com.


          8. It would be nice to use the user's name instead of User when displaying the winner and when displaying choices.



          The following points may directly contradict what your teacher says, as they are more opinion based. Follow whatever your team's (or assignment's) guidelines say.





          1. Don't be afraid to include more than one line of code in main. If I wrote this program my main function would look something like this:



            Scanner scanner = new Scanner(System.in);
            System.out.println("Please enter your name:");

            Player user = new User(scanner.nextLine());
            Player computer = new Computer();
            Game game = new Game(user, computer);

            do {
            game.play();
            System.out.println("Play again? [Yes/No]");
            } while (scanner.nextLine().toUpperCase().equals("YES"));


          2. Use braces on all if statements that aren't one line long. I would be fine with if (winner == 0) playerWins++;, but if (winner == 0)n playerWins++; is much easier to mess up, especially if you don't have automatic indentation.


          3. Don't declare variables before initializing them, if possible. There's no reason to declare choice before printing out instructions in the playAgain method.







          share|improve this answer












          This code really isn't a mess! While there are a few relatively minor points I can make, your code is generally easy to read and understand, and there's nothing horribly wrong about it.




          1. Watch out for user input! I can enter Z (or anything but rock/paper/scissors) and the result will always be a tie. You should tell the user when they enter something invalid and have them correct it.


          2. Both Computer and Player use setName to set their name - once. I would prefer requiring the use of the Player constructor with the name parameter since you could then mark name as final.


          3. Why is setChoice public? It is (and should) only used by selectChoice, so mark it protected.


          4. What happens when I want to play a game between two computer opponents (or two people)? Right now I can't. It would be nice if Game let me pass the two Player objects in that will be opponents.


          5. While I appreciate the MAX_NUMBER constant in the Computer class instead of just including a magic number, it's a better idea to store your choices in an array and then select a random element of the array.



          6. Passing around strings for choices isn't so bad when just dealing with Rock/Paper/Scissors, but it will quickly become unmanagable. Enum Types were introduced to fix this problem, and generally do a pretty good job of it! It wouldn't hurt to start using them now. Another good spot for an enumeration is in the decideWinner method.



            The Choice enum type could also provide a method winsAgainst(Choice other) that can be used to simplify decideWinner.



          7. For the most part, your names are great... until we get to Game and see p and com.


          8. It would be nice to use the user's name instead of User when displaying the winner and when displaying choices.



          The following points may directly contradict what your teacher says, as they are more opinion based. Follow whatever your team's (or assignment's) guidelines say.





          1. Don't be afraid to include more than one line of code in main. If I wrote this program my main function would look something like this:



            Scanner scanner = new Scanner(System.in);
            System.out.println("Please enter your name:");

            Player user = new User(scanner.nextLine());
            Player computer = new Computer();
            Game game = new Game(user, computer);

            do {
            game.play();
            System.out.println("Play again? [Yes/No]");
            } while (scanner.nextLine().toUpperCase().equals("YES"));


          2. Use braces on all if statements that aren't one line long. I would be fine with if (winner == 0) playerWins++;, but if (winner == 0)n playerWins++; is much easier to mess up, especially if you don't have automatic indentation.


          3. Don't declare variables before initializing them, if possible. There's no reason to declare choice before printing out instructions in the playAgain method.








          share|improve this answer












          share|improve this answer



          share|improve this answer










          answered 1 hour ago









          Gerrit0

          2,9291522




          2,9291522
























              up vote
              1
              down vote













              After writing this review, I noticed there is still a few things that there were a some improvements that would require more-advanced-than-beginner levels of Java syntax. This answers focuses mostly on language agnostic improvements. There are some other things that can be added, but I really wanted to focus on the idea of a "Game Loop" before getting into too many details.



              Game loop



              You don't follow a typical game loop. Here is an article about creating a game loop. Fortunately, you don't need to worry about Frames Per Second (FPS) interpolation which is a large part of the article I linked. Nevertheless, it is imperative to look at this:



              bool game_is_running = true;

              while( game_is_running ) {
              update_game();
              display_game();
              }


              This logic should be in you main method. In other words, something like:



              public class Main {
              public static void main(String args) {
              Game game = new Game();
              game.start();
              while (game.isRunning()) {
              game.step();
              game.display();
              }
              }
              }


              Refactoring start



              Thus, it is imperative that we refactor this start method you have. First, make it public so I can call it as such. (There are actually a couple of other things related to starting, ending, and restarting games that you should probably do, but I think there is already enough in this CodeReview.)



              What is step?



              The step method mentioned above is actually located in your start. It is almost:



              public void step() {
              displayScore();
              p.selectChoice();
              com.selectChoice();
              displayChoices();
              displayWinner(decideWinner());
              updateScore(decideWinner());
              playAgain();
              }


              We need still change a couple things:




              1. This is more of stepAndDisplay not step.

              2. We will need to make some changes about the Player class (more on that later).


              Let's break step and display up.



              step should generate the data, and handle input. display should output the data. So:



              public void display() {
              displayOldScore();
              displayChoices();
              displayWinner();
              displayNewScore();
              playAgain();
              }


              And step could be:



              public void step() {
              oldScore = newScore;
              p.selectChoice();
              com.selectChoice();
              newScore += decideWinner();
              }


              Refactoring step even further.



              Instead of what I just wrote, the selectChoice() method is a bit awkward, I would just want to do:



              public void step() {
              oldScore = newScore;
              newScore += decideWinner(p.getChoice(), com.getChoice());
              }


              But, even this can worked on:




              Player class



              Now back to the idea of selectChoice: Just implement getChoice as what is currently selectChoice. For example the Computer method selectChoice becomes:



              public String getChoice() {
              int randomNumber = rand.nextInt(MAX_NUMBER);
              switch(randomNumber) {
              case 0:
              return "ROCK";
              case 1:
              return "PAPER";
              case 2:
              return "SCISSORS";
              }
              }


              Handling different player configurations



              Currently, you cannot have two human players play each other. I would imagine you want this option.



              Here are some changes that would need to be made in order for this to happen:



              // Each player should have type player, don't make it `User` and `Computer`.
              private Player p1;
              private Player p2;

              public Game() { // Default...
              p1 = new User();
              p2 = new Computer();
              scan = new Scanner(System.in);
              }

              public Game(Player firstPlayer, Player secondPlayer) {
              p1 = firstPlayer;
              p2 = secondPlayer;
              scan = new Scanner(System.in);
              }


              (There are further changes that I leave to you as an exercise: How do you change displayScore?)




              Computer class



              Your computer class is a particular kind of AI. Your Computer implements a "random selection" so I would change the class name to RandomComputer or something like that. Bottom line: You may want other computers in the future.





              1. However You need an end game function too as well as some other additions.






              share|improve this answer



























                up vote
                1
                down vote













                After writing this review, I noticed there is still a few things that there were a some improvements that would require more-advanced-than-beginner levels of Java syntax. This answers focuses mostly on language agnostic improvements. There are some other things that can be added, but I really wanted to focus on the idea of a "Game Loop" before getting into too many details.



                Game loop



                You don't follow a typical game loop. Here is an article about creating a game loop. Fortunately, you don't need to worry about Frames Per Second (FPS) interpolation which is a large part of the article I linked. Nevertheless, it is imperative to look at this:



                bool game_is_running = true;

                while( game_is_running ) {
                update_game();
                display_game();
                }


                This logic should be in you main method. In other words, something like:



                public class Main {
                public static void main(String args) {
                Game game = new Game();
                game.start();
                while (game.isRunning()) {
                game.step();
                game.display();
                }
                }
                }


                Refactoring start



                Thus, it is imperative that we refactor this start method you have. First, make it public so I can call it as such. (There are actually a couple of other things related to starting, ending, and restarting games that you should probably do, but I think there is already enough in this CodeReview.)



                What is step?



                The step method mentioned above is actually located in your start. It is almost:



                public void step() {
                displayScore();
                p.selectChoice();
                com.selectChoice();
                displayChoices();
                displayWinner(decideWinner());
                updateScore(decideWinner());
                playAgain();
                }


                We need still change a couple things:




                1. This is more of stepAndDisplay not step.

                2. We will need to make some changes about the Player class (more on that later).


                Let's break step and display up.



                step should generate the data, and handle input. display should output the data. So:



                public void display() {
                displayOldScore();
                displayChoices();
                displayWinner();
                displayNewScore();
                playAgain();
                }


                And step could be:



                public void step() {
                oldScore = newScore;
                p.selectChoice();
                com.selectChoice();
                newScore += decideWinner();
                }


                Refactoring step even further.



                Instead of what I just wrote, the selectChoice() method is a bit awkward, I would just want to do:



                public void step() {
                oldScore = newScore;
                newScore += decideWinner(p.getChoice(), com.getChoice());
                }


                But, even this can worked on:




                Player class



                Now back to the idea of selectChoice: Just implement getChoice as what is currently selectChoice. For example the Computer method selectChoice becomes:



                public String getChoice() {
                int randomNumber = rand.nextInt(MAX_NUMBER);
                switch(randomNumber) {
                case 0:
                return "ROCK";
                case 1:
                return "PAPER";
                case 2:
                return "SCISSORS";
                }
                }


                Handling different player configurations



                Currently, you cannot have two human players play each other. I would imagine you want this option.



                Here are some changes that would need to be made in order for this to happen:



                // Each player should have type player, don't make it `User` and `Computer`.
                private Player p1;
                private Player p2;

                public Game() { // Default...
                p1 = new User();
                p2 = new Computer();
                scan = new Scanner(System.in);
                }

                public Game(Player firstPlayer, Player secondPlayer) {
                p1 = firstPlayer;
                p2 = secondPlayer;
                scan = new Scanner(System.in);
                }


                (There are further changes that I leave to you as an exercise: How do you change displayScore?)




                Computer class



                Your computer class is a particular kind of AI. Your Computer implements a "random selection" so I would change the class name to RandomComputer or something like that. Bottom line: You may want other computers in the future.





                1. However You need an end game function too as well as some other additions.






                share|improve this answer

























                  up vote
                  1
                  down vote










                  up vote
                  1
                  down vote









                  After writing this review, I noticed there is still a few things that there were a some improvements that would require more-advanced-than-beginner levels of Java syntax. This answers focuses mostly on language agnostic improvements. There are some other things that can be added, but I really wanted to focus on the idea of a "Game Loop" before getting into too many details.



                  Game loop



                  You don't follow a typical game loop. Here is an article about creating a game loop. Fortunately, you don't need to worry about Frames Per Second (FPS) interpolation which is a large part of the article I linked. Nevertheless, it is imperative to look at this:



                  bool game_is_running = true;

                  while( game_is_running ) {
                  update_game();
                  display_game();
                  }


                  This logic should be in you main method. In other words, something like:



                  public class Main {
                  public static void main(String args) {
                  Game game = new Game();
                  game.start();
                  while (game.isRunning()) {
                  game.step();
                  game.display();
                  }
                  }
                  }


                  Refactoring start



                  Thus, it is imperative that we refactor this start method you have. First, make it public so I can call it as such. (There are actually a couple of other things related to starting, ending, and restarting games that you should probably do, but I think there is already enough in this CodeReview.)



                  What is step?



                  The step method mentioned above is actually located in your start. It is almost:



                  public void step() {
                  displayScore();
                  p.selectChoice();
                  com.selectChoice();
                  displayChoices();
                  displayWinner(decideWinner());
                  updateScore(decideWinner());
                  playAgain();
                  }


                  We need still change a couple things:




                  1. This is more of stepAndDisplay not step.

                  2. We will need to make some changes about the Player class (more on that later).


                  Let's break step and display up.



                  step should generate the data, and handle input. display should output the data. So:



                  public void display() {
                  displayOldScore();
                  displayChoices();
                  displayWinner();
                  displayNewScore();
                  playAgain();
                  }


                  And step could be:



                  public void step() {
                  oldScore = newScore;
                  p.selectChoice();
                  com.selectChoice();
                  newScore += decideWinner();
                  }


                  Refactoring step even further.



                  Instead of what I just wrote, the selectChoice() method is a bit awkward, I would just want to do:



                  public void step() {
                  oldScore = newScore;
                  newScore += decideWinner(p.getChoice(), com.getChoice());
                  }


                  But, even this can worked on:




                  Player class



                  Now back to the idea of selectChoice: Just implement getChoice as what is currently selectChoice. For example the Computer method selectChoice becomes:



                  public String getChoice() {
                  int randomNumber = rand.nextInt(MAX_NUMBER);
                  switch(randomNumber) {
                  case 0:
                  return "ROCK";
                  case 1:
                  return "PAPER";
                  case 2:
                  return "SCISSORS";
                  }
                  }


                  Handling different player configurations



                  Currently, you cannot have two human players play each other. I would imagine you want this option.



                  Here are some changes that would need to be made in order for this to happen:



                  // Each player should have type player, don't make it `User` and `Computer`.
                  private Player p1;
                  private Player p2;

                  public Game() { // Default...
                  p1 = new User();
                  p2 = new Computer();
                  scan = new Scanner(System.in);
                  }

                  public Game(Player firstPlayer, Player secondPlayer) {
                  p1 = firstPlayer;
                  p2 = secondPlayer;
                  scan = new Scanner(System.in);
                  }


                  (There are further changes that I leave to you as an exercise: How do you change displayScore?)




                  Computer class



                  Your computer class is a particular kind of AI. Your Computer implements a "random selection" so I would change the class name to RandomComputer or something like that. Bottom line: You may want other computers in the future.





                  1. However You need an end game function too as well as some other additions.






                  share|improve this answer














                  After writing this review, I noticed there is still a few things that there were a some improvements that would require more-advanced-than-beginner levels of Java syntax. This answers focuses mostly on language agnostic improvements. There are some other things that can be added, but I really wanted to focus on the idea of a "Game Loop" before getting into too many details.



                  Game loop



                  You don't follow a typical game loop. Here is an article about creating a game loop. Fortunately, you don't need to worry about Frames Per Second (FPS) interpolation which is a large part of the article I linked. Nevertheless, it is imperative to look at this:



                  bool game_is_running = true;

                  while( game_is_running ) {
                  update_game();
                  display_game();
                  }


                  This logic should be in you main method. In other words, something like:



                  public class Main {
                  public static void main(String args) {
                  Game game = new Game();
                  game.start();
                  while (game.isRunning()) {
                  game.step();
                  game.display();
                  }
                  }
                  }


                  Refactoring start



                  Thus, it is imperative that we refactor this start method you have. First, make it public so I can call it as such. (There are actually a couple of other things related to starting, ending, and restarting games that you should probably do, but I think there is already enough in this CodeReview.)



                  What is step?



                  The step method mentioned above is actually located in your start. It is almost:



                  public void step() {
                  displayScore();
                  p.selectChoice();
                  com.selectChoice();
                  displayChoices();
                  displayWinner(decideWinner());
                  updateScore(decideWinner());
                  playAgain();
                  }


                  We need still change a couple things:




                  1. This is more of stepAndDisplay not step.

                  2. We will need to make some changes about the Player class (more on that later).


                  Let's break step and display up.



                  step should generate the data, and handle input. display should output the data. So:



                  public void display() {
                  displayOldScore();
                  displayChoices();
                  displayWinner();
                  displayNewScore();
                  playAgain();
                  }


                  And step could be:



                  public void step() {
                  oldScore = newScore;
                  p.selectChoice();
                  com.selectChoice();
                  newScore += decideWinner();
                  }


                  Refactoring step even further.



                  Instead of what I just wrote, the selectChoice() method is a bit awkward, I would just want to do:



                  public void step() {
                  oldScore = newScore;
                  newScore += decideWinner(p.getChoice(), com.getChoice());
                  }


                  But, even this can worked on:




                  Player class



                  Now back to the idea of selectChoice: Just implement getChoice as what is currently selectChoice. For example the Computer method selectChoice becomes:



                  public String getChoice() {
                  int randomNumber = rand.nextInt(MAX_NUMBER);
                  switch(randomNumber) {
                  case 0:
                  return "ROCK";
                  case 1:
                  return "PAPER";
                  case 2:
                  return "SCISSORS";
                  }
                  }


                  Handling different player configurations



                  Currently, you cannot have two human players play each other. I would imagine you want this option.



                  Here are some changes that would need to be made in order for this to happen:



                  // Each player should have type player, don't make it `User` and `Computer`.
                  private Player p1;
                  private Player p2;

                  public Game() { // Default...
                  p1 = new User();
                  p2 = new Computer();
                  scan = new Scanner(System.in);
                  }

                  public Game(Player firstPlayer, Player secondPlayer) {
                  p1 = firstPlayer;
                  p2 = secondPlayer;
                  scan = new Scanner(System.in);
                  }


                  (There are further changes that I leave to you as an exercise: How do you change displayScore?)




                  Computer class



                  Your computer class is a particular kind of AI. Your Computer implements a "random selection" so I would change the class name to RandomComputer or something like that. Bottom line: You may want other computers in the future.





                  1. However You need an end game function too as well as some other additions.







                  share|improve this answer














                  share|improve this answer



                  share|improve this answer








                  edited 52 mins ago

























                  answered 58 mins ago









                  Dair

                  4,350727




                  4,350727






















                      Žiga Černič is a new contributor. Be nice, and check out our Code of Conduct.










                      draft saved

                      draft discarded


















                      Žiga Černič is a new contributor. Be nice, and check out our Code of Conduct.













                      Žiga Černič is a new contributor. Be nice, and check out our Code of Conduct.












                      Žiga Černič is a new contributor. Be nice, and check out our Code of Conduct.
















                      Thanks for contributing an answer to Code Review Stack Exchange!


                      • Please be sure to answer the question. Provide details and share your research!

                      But avoid



                      • Asking for help, clarification, or responding to other answers.

                      • Making statements based on opinion; back them up with references or personal experience.


                      Use MathJax to format equations. MathJax reference.


                      To learn more, see our tips on writing great answers.





                      Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


                      Please pay close attention to the following guidance:


                      • Please be sure to answer the question. Provide details and share your research!

                      But avoid



                      • Asking for help, clarification, or responding to other answers.

                      • Making statements based on opinion; back them up with references or personal experience.


                      To learn more, see our tips on writing great answers.




                      draft saved


                      draft discarded














                      StackExchange.ready(
                      function () {
                      StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209331%2frock-paper-scissors-game-using-oop%23new-answer', 'question_page');
                      }
                      );

                      Post as a guest















                      Required, but never shown





















































                      Required, but never shown














                      Required, but never shown












                      Required, but never shown







                      Required, but never shown

































                      Required, but never shown














                      Required, but never shown












                      Required, but never shown







                      Required, but never shown







                      Popular posts from this blog

                      What visual should I use to simply compare current year value vs last year in Power BI desktop

                      How to ignore python UserWarning in pytest?

                      Alexandru Averescu