Skip to content
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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file not shown.
Binary file not shown.
Binary file added .gradle/6.8.1/fileChanges/last-build.bin
Binary file not shown.
Binary file added .gradle/6.8.1/fileHashes/fileHashes.bin
Binary file not shown.
Binary file added .gradle/6.8.1/fileHashes/fileHashes.lock
Binary file not shown.
Empty file added .gradle/6.8.1/gc.properties
Empty file.
Binary file added .gradle/6.8.1/javaCompile/classAnalysis.bin
Binary file not shown.
Binary file added .gradle/6.8.1/javaCompile/javaCompile.lock
Binary file not shown.
Binary file added .gradle/6.8.1/javaCompile/taskHistory.bin
Binary file not shown.
Binary file added .gradle/buildOutputCleanup/buildOutputCleanup.lock
Binary file not shown.
2 changes: 2 additions & 0 deletions .gradle/buildOutputCleanup/cache.properties
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
Binary file added .gradle/buildOutputCleanup/outputFiles.bin
Binary file not shown.
Binary file added .gradle/checksums/checksums.lock
Binary file not shown.
Binary file added .gradle/checksums/md5-checksums.bin
Binary file not shown.
Binary file added .gradle/checksums/sha1-checksums.bin
Binary file not shown.
Empty file.
Empty file added .gradle/vcs-1/gc.properties
Empty file.
3 changes: 3 additions & 0 deletions .idea/.gitignore

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions .idea/.name

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions .idea/codeStyles/Project.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions .idea/codeStyles/codeStyleConfig.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/compiler.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions .idea/gradle.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions .idea/jarRepositories.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions .idea/libraries/netty_all_4_1_18_Final.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions .idea/misc.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions .idea/vcs.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,8 @@ repositories {

dependencies {
testImplementation group: 'junit', name: 'junit', version: '4.13.1'
// https://mvnrepository.com/artifact/io.netty/netty-all
implementation group: 'io.netty', name: 'netty-all', version: '4.1.71.Final'
// https://mvnrepository.com/artifact/io.socket/socket.io-client
implementation group: 'io.socket', name: 'socket.io-client', version: '2.0.1'
}
Binary file removed build/classes/java/main/Main.class
Binary file not shown.
Binary file not shown.
Binary file added build/classes/java/main/softwaredesign/Main.class
Binary file not shown.
4 changes: 4 additions & 0 deletions build/tmp/compileJava/source-classes-mapping.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
softwaredesign/Main.java
softwaredesign.Main
softwaredesign/CommandHandler.java
softwaredesign.CommandHandler
Binary file added docs/Assignment 2.pdf
Binary file not shown.
Binary file added lib/netty-all-4.1.18.Final.jar
Binary file not shown.
Binary file not shown.
Binary file not shown.
3 changes: 3 additions & 0 deletions src/main/java/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Manifest-Version: 1.0
Main-Class: softwaredesign.Main

76 changes: 76 additions & 0 deletions src/main/java/softwaredesign/CommandHandler.java
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;
Comment on lines +8 to +9

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be private.

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.

Copy link

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.

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.

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.

public CommandHandler() {
games = new HashMap<>();
gameSequence = new ArrayList<String>();
}
Comment on lines +10 to +13

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is only one central CommandHandler defined in Main you could consider making this a singleton object.

Copy link

Choose a reason for hiding this comment

The 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(" ");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is repeated a lot, consider splitting the string in Main and passing the array to the individual handlers.

Copy link

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already handled in Main.

Copy link

Choose a reason for hiding this comment

The 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");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These error messages could be replaced by constants (e.g. String INVALID_COMMAND_MESSAGE = "Invalid command"). If you then ever want to change the message you won't have to change it everywhere where this message is repeated.

return;
}
String gameName = commandSplitted[2];
String state = commandSplitted[3];
if (!(state.equals("PLAY") || state.equals("PAUSED") || state.equals("FINISHED"))) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add a list stateOptions that contains these strings. Then this if statement could be simplified to if(!stateOptions.contains(state)) { ... }.

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) {

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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];

Choose a reason for hiding this comment

The 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);

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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) {

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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 Command class would be interesting with a function to parse the String into a Command object (that stores the parameters and opcode) or something similar.

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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++) {

Choose a reason for hiding this comment

The 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) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArrayList has .isEmpty().

System.out.println("No game sequence set yet");
return;
}
System.out.println("Current game sequence: ");
for(int i = 0; i < gameSequence.toArray().length; i++) {

Choose a reason for hiding this comment

The 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 ArrayList using .size().

System.out.println((i + 1) + ": " + gameSequence.get(i));
}
}
}

Choose a reason for hiding this comment

The 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.

26 changes: 23 additions & 3 deletions src/main/java/softwaredesign/Main.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,27 @@
package softwaredesign;
Copy link

Choose a reason for hiding this comment

The 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");

Choose a reason for hiding this comment

The 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) {

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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 main()? In my opinion it could go into the CommandHandler class, or a CLIHandler or something similar.

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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?)

Choose a reason for hiding this comment

The 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")) {

Choose a reason for hiding this comment

The 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")) {

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing commands
You parse by getting an input string and checking if this string contains a specific command. This is error-prone as the contains() method does not capture small errors with whitespace or capitalization (like "SET GAMESTATE" or "set GAMESTATE" would not be recognized as valid commands).
If you want to use the contains() method, I would suggest converting your command String from line 12 to uppercase first using Java's String.toUpperCase() method. This would resolve capitalization errors. To further improve, you can split your raw command String into an array of Strings. If you split the String on whitespace, you can easily parse commands and eliminate parsing errors due to incorrect whitespace.

Naming and separation of concerns
It is not really clear why you parse commands in Main when you also have a CommandHandler class. I would suggest making a CLI or similar named class in which you do all the parsing. If you move your functionality from the current CommandHandler class to a specific Game class, you can do all parsing and command handling in the CLI class and let this CLI class call the appropriate methods in your Game class. In my opinion separation of concerns is covered better this way.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the command parsing, you split on whitespace repetitively in your CommandHandler methods, as per @petaripetrov's comment, consider centralizing this in main or in the suggested CLI class.

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this should be in Main. Maybe a function in CommandHandler that does this?

}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure you close your Scanner properly.

}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks very good. Code was clear and readable, easy to understand.

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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.
The only thing I can add is that personally, I like writing short comments on my codes such as at the beggining of the classes/functions etc. They make me understand the reason better why I use sth in there, so maybe you can try that. But as I said, it is something that I like personally, so it is up to you! Good luck in your project