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

Basic Rock-Paper-Scissors implementation

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

Problem

I decided to try to make a simple RPS implementation in Java 8, the following notes should be said first:

  • At a later point it should support RPSLS aswell, as any other Gesture-game variant.



  • At a later point it should support different views, for example a graphical one.



  • I have made a special ConsoleReader class which allows me to kill reading from System.in without closing the stream.



I would like comments on:

  • Everything



  • Concurrency issues



  • Variable, method and javadoc naming and wording



The code should be quite self-explanatory.

```
/**
* Class that provides a console reader, from which other classes can read.
*
* @author Frank van Heeswijk
*/
public class ConsoleReader implements Runnable {
/** Synchronized mapping from objects to a list of string consumers to consume input lines. */
private final static Map>> CONSUMER_MAP = Collections.synchronizedMap(new HashMap<>());

/** Synchronized mapping from objects to a blocking queue of strings to read input lines while maintaining blocking behaviour. */
private final static Map> BLOCKING_QUEUE_MAP = Collections.synchronizedMap(new HashMap<>());

/**
* Adds a consumer on the object you pass. When console input happens, the consumer will be triggered.
*
* @param object The object to which the consumers belong
* @param consumer The consumer to handle new input
*/
public static void addConsumer(final Object object, final Consumer consumer) {
ensureKey(object, CONSUMER_MAP, ArrayList::new);
CONSUMER_MAP.get(object).add(consumer);
}

/**
* Removes all consumers related to the object you pass.
*
* @param object The object to which consumers might be attached
*/
public static void removeConsumers(final Object object) {
CONSUMER_MAP.remove(object);
}

@Override
public void run() {
Scanner scanner = new Scanner(System.in);
while (scanner.hasNextLine()) {

Solution

-
I like your usage of Java 8. I'm learning a lot just by reading your code. I'm loving Java 8 a lot and I haven't even used it myself yet.

-
Use a GestureRule class. Take a look at The Mug's Main class and/or The Monkey's static initializer in his enum. Your current way is error prone and causes some duplicated code. Currently you can by accident create a Gesture that both wins, loses and ties another gesture. That really shouldn't be possible. I'd recommend using a method such as gesture.fight(otherGesture) that returns a RPSResult to make sure that a fight between one gesture and another cannot be both a win and a loss at the same time.

-
You're currently not using the onPostAction method, and I do wonder whether you're gonna need it at all. And if you are going to need it, it can likely be made in a different/better way. By getting rid of it / doing it differently you also get rid of the R extends Result generic part of your Player interface/class(es). An option would be to use onPostRound(Map gestures) (using only the 'A' and 'P' generics) and do a for-each on the gestures you're interested in. Because technically, players make their moves at the same time.

-
Your Action and Result interfaces only seems to be marker interfaces, you don't really need them. If you remove them it will decrease your generic dependencies and you can write:

public interface Game> {


-
Interface methods doesn't need to be declared public, they're public automatically.

-
In your playGame method, why do you declare a boolean variable instead of using while (true)?

-
You're using newFixedThreadPool which means that your number of threads cannot go above the specified amount. Instead consider using a cached thread pool which will create as many threads as needed while keeping a specified number of threads whether they're busy or not. By using a fixed thread pool you will likely have several threads that will never have anything to do.

-
Use a toString method in your different RPSPlayer classes or a player.isAI() method instead of using instanceof in your ConsoleRPSView. Using instanceof outside an .equals method is not good practice, it can mostly be avoided. Use polymorphism and better interfaces instead.

System.out.println(player + " played " + gesture + " and the result was a " + result);
...
if (!player.isAI())
    System.out.println("You have not entered your gesture in time.");


-
Typo: System.out.println("Your gesture was nog recognized - nog --> not. 'nog' means 'probably' in Swedish so if reading this in a combined English-Swedish way it'd be quite funny...

-
Your RPSGameMain.init method does more than just initializing, doesn't it? I'd recommend either splitting into two methods or rename it to start or similar.

-
Overall your code quality is good, it is easy to read and mostly easy to understand.

-
I like the way you've decoupled your "console view" from your "model" with an interface :)

Code Snippets

public interface Game<A, R, P extends Player<A, R>> {
System.out.println(player + " played " + gesture + " and the result was a " + result);
...
if (!player.isAI())
    System.out.println("You have not entered your gesture in time.");

Context

StackExchange Code Review Q#45015, answer score: 12

Revisions (0)

No revisions yet.