-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Assignment 2 #2
base: master
Are you sure you want to change the base?
Assignment 2 #2
Changes from all commits
e585d44
b357ce9
475b13c
3748553
90705e2
48571d6
93bc6e2
ec65a6d
ba17cd7
a2615ed
4db6ab0
314c5e2
832668b
ff1f197
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
#Mon Feb 07 12:09:45 CET 2022 | ||
gradle.version=6.8.1 |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
softwaredesign/Main.java | ||
softwaredesign.Main | ||
softwaredesign/CommandHandler.java | ||
softwaredesign.CommandHandler |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Manifest-Version: 1.0 | ||
Main-Class: softwaredesign.Main | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
package softwaredesign; | ||
|
||
import java.util.ArrayList; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
public class CommandHandler { | ||
Map<String, String> games; | ||
ArrayList<String> gameSequence; | ||
public CommandHandler() { | ||
games = new HashMap<>(); | ||
gameSequence = new ArrayList<String>(); | ||
} | ||
Comment on lines
+10
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there is only one central There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless your going to use combinations of commands with multiple instances of the game as an example, a singleton may make your intentions on how your going to use the program more clear. |
||
|
||
protected void setGameState(String command) { | ||
String[] commandSplitted = command.split(" "); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is repeated a lot, consider splitting the string in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. String[] commandSplitted = command.split(" "); And String gameName = commandSplitted[2]; Are repeated for all instances, so these could be generalized. The other splits are more specific so thats ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Splitting of command is repeated code, and can be handled before this function (possibly combined with the if statements in main) |
||
if (commandSplitted.length != 4) { | ||
if(commandSplitted[0].equals("EXIT") && commandSplitted.length == 1){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is already handled in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part should indeed be redundant, if it isn't then there is something wrong with your if statements in main, or with your while loop. |
||
System.out.println("Exiting control room"); | ||
System.exit(0); | ||
} | ||
System.out.println("Invalid command"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These error messages could be replaced by constants (e.g. |
||
return; | ||
} | ||
String gameName = commandSplitted[2]; | ||
String state = commandSplitted[3]; | ||
if (!(state.equals("PLAY") || state.equals("PAUSED") || state.equals("FINISHED"))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could add a list |
||
System.out.println("Invalid gamestate"); | ||
return; | ||
} | ||
games.put(gameName, state); | ||
System.out.println("Set the game " + gameName + " to " + state); | ||
} | ||
|
||
protected void getGameState(String command) { | ||
String[] commandSplitted = command.split(" "); | ||
if (commandSplitted.length > 3) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the length is less than 3 it will continue and get an index out of bounds error in line 41. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't there a return at line 39? It should just reenter the loop in main. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah there is indeed a return, so it'll return to the main loop. |
||
System.out.println("Invalid command"); | ||
return; | ||
} | ||
String gameName = commandSplitted[2]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using newlines between blocks of code that handle different things |
||
String state = games.get(gameName); | ||
if (state == null) { | ||
System.out.println("The game " + gameName + " does not exist!"); | ||
return; | ||
} | ||
System.out.println("The gamestate of " + gameName + " is " + state); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of the early return, it could have just been an if... else block. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should actually omit as many elses as possible, so that would be a bad practice. |
||
} | ||
|
||
protected void setGameSequence(String command) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you call this function multiple times it will keep adding to the same game sequence rather than "setting" it to the parameters. Is this intended? If so, the name of the function is a little bit misleading. |
||
String[] commandSplitted = command.split(" "); | ||
if (commandSplitted.length < 3) { | ||
System.out.println("Invalid command"); | ||
return; | ||
} | ||
Comment on lines
+51
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is repeated in a lot of the functions. Perhaps some kind of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is repeated code, a method to safely split the command could avoid this.
Comment on lines
+51
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As previously mentioned the code is repeated. |
||
for(int i = 2; i < commandSplitted.length; i++) { | ||
gameSequence.add(commandSplitted[i]); | ||
} | ||
|
||
System.out.println("Set the game sequence to:"); | ||
for(int i = 0; i < gameSequence.toArray().length; i++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can use .size() instead of converting to an array. |
||
System.out.println((i + 1) + ": " + gameSequence.get(i)); | ||
} | ||
} | ||
|
||
protected void getGameSequence() { | ||
if (gameSequence.toArray().length == 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
System.out.println("No game sequence set yet"); | ||
return; | ||
} | ||
System.out.println("Current game sequence: "); | ||
for(int i = 0; i < gameSequence.toArray().length; i++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of converting to an array you can just get the length of the |
||
System.out.println((i + 1) + ": " + gameSequence.get(i)); | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall the functionality of the command handler looks good, especially if you include the if-else block in main inside this file. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,27 @@ | ||
package softwaredesign; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general your commandhandler class is sufficiently deep with simple functionality, so that is good. And the idea of ielajazdeev of making input commands generalized by first capitalizing them to avoid errors is a simple implementation to make your system more bug proof. Also, as Woutuuur commented you can save on lines by avoiding reusing the same code by making a class to handle strings for example. Finally, right now if you enter an invalid command you don't get feedback. There are also no instructions to operate the system. It also only responds to capitalized letters. |
||
|
||
import java.util.Scanner; | ||
|
||
public class Main { | ||
public static void main (String[] args){ | ||
System.out.println("Welcome to Software Design"); | ||
public static void main (String[] args) { | ||
CommandHandler commandHandler = new CommandHandler(); | ||
System.out.println("Welcome to the Squid Game Control Room"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is best practice to hide string literals behind constants for reusability, so you can make this more reusable by using a constant with the value of the string literal |
||
while (true) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The entire while block can be formatted better, as right now it is hard to read There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This loop is the CLI prompt which handles the input of commands. Should this really be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that the while loop could be better formatted for better readability and the if statements can be a separate block. Also I would personally format the else if statements differently but I guess it is based on one's choice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The command handler can possibly include this functionality that is now in main, but that's a matter of taste. Could be formatted differently as well (maybe a switch statement?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am also not sure if this part must be in main or not |
||
System.out.print("> "); | ||
Scanner scanner = new Scanner(System.in); | ||
String command = scanner.nextLine(); | ||
if(command.contains("SET GAMESTATE")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also be abstracted behind constants |
||
commandHandler.setGameState(command); | ||
} else if(command.contains("GET GAMESTATE")) { | ||
commandHandler.getGameState(command); | ||
} else if(command.contains("SET GAMESEQUENCE")) { | ||
commandHandler.setGameSequence(command); | ||
}else if(command.contains("GET GAMESEQUENCE")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small indentation mistake |
||
commandHandler.getGameSequence(); | ||
} else if(command.contains("EXIT")) { | ||
Comment on lines
+11
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parsing commands Naming and separation of concerns There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding the command parsing, you split on whitespace repetitively in your There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Case sensitive commands might not be the best choice. Consider converting strings to upper / lower case before parsing them. |
||
System.out.println("Exited the Squid Game Control Room"); | ||
break; | ||
} | ||
Comment on lines
+13
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this should be in |
||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure you close your |
||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall looks very good. Code was clear and readable, easy to understand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Presently the app works in a command line interface but if it had to be extended to a gui it would have to be redone. Maybe it is a good idea to separate command line outputs with functions. So that regardless of implementation the main function need not be changed that much. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most comments I made have already been made. Clear readable code, got nothing else to add. With the slight improvements it should be good for further implementation in assignment 3. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of things were already written, so I did not add them again.Other than these small issues, I think you have done it really well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And they can also be final.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how gamesequence may be used for other things in the future hence the public class. Otherwise why not private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear naming of variables. Package and Import statements consistent with google guidelines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Readable code, well formatted and follows good naming convention.