patternjavaModerate
Basic Rock-Paper-Scissors implementation
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:
I would like comments on:
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()) {
- 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
ConsoleReaderclass which allows me to kill reading fromSystem.inwithout 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
-
You're currently not using the
-
Your
-
Interface methods doesn't need to be declared
-
In your
-
You're using
-
Use a
-
Typo:
-
Your
-
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 :)
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.