Assignment 2 #2

Jaystah · 2022-03-11T18:50:53Z

No description provided.

Woutuuur · 2022-03-19T11:41:42Z

src/main/java/softwaredesign/CommandHandler.java

+
+    protected void getGameState(String command) {
+        String[] commandSplitted = command.split(" ");
+        if (commandSplitted.length > 3) {


If the length is less than 3 it will continue and get an index out of bounds error in line 41.

Isn't there a return at line 39? It should just reenter the loop in main.

Yeah there is indeed a return, so it'll return to the main loop.

Woutuuur · 2022-03-19T11:48:07Z

src/main/java/softwaredesign/CommandHandler.java

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


Instead of converting to an array you can just get the length of the ArrayList using .size().

Woutuuur · 2022-03-19T11:52:23Z

src/main/java/softwaredesign/CommandHandler.java

+                System.out.println("Exiting control room");
+                System.exit(0);
+            }
+            System.out.println("Invalid command");


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.

Woutuuur · 2022-03-19T11:56:49Z

src/main/java/softwaredesign/CommandHandler.java

+    protected void setGameState(String command) {
+        String[] commandSplitted = command.split(" ");
+        if (commandSplitted.length != 4) {
+            if(commandSplitted[0].equals("EXIT") && commandSplitted.length == 1){


This is already handled in Main.

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.

Woutuuur · 2022-03-19T12:04:46Z

src/main/java/softwaredesign/CommandHandler.java

+        System.out.println("The gamestate of " + gameName + " is " + state);
+    }
+
+    protected void setGameSequence(String command) {


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.

Woutuuur · 2022-03-19T12:07:57Z

src/main/java/softwaredesign/CommandHandler.java

+    Map<String, String> games;
+    ArrayList<String> gameSequence;


These can be private.

And they can also be final.

I can see how gamesequence may be used for other things in the future hence the public class. Otherwise why not private.

Clear naming of variables. Package and Import statements consistent with google guidelines.

Readable code, well formatted and follows good naming convention.

Woutuuur · 2022-03-19T12:08:39Z

src/main/java/softwaredesign/CommandHandler.java

+        String[] commandSplitted = command.split(" ");
+        if (commandSplitted.length < 3) {
+            System.out.println("Invalid command");
+            return;
+        }


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.

Woutuuur · 2022-03-19T12:14:34Z

src/main/java/softwaredesign/CommandHandler.java

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


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

Woutuuur · 2022-03-19T12:16:27Z

src/main/java/softwaredesign/Main.java

+                commandHandler.getGameState(command);
+            } else if(command.contains("SET GAMESEQUENCE")) {
+                commandHandler.setGameSequence(command);
+            }else if(command.contains("GET GAMESEQUENCE")) {


Small indentation mistake

Woutuuur · 2022-03-19T12:21:25Z

src/main/java/softwaredesign/Main.java

+            if(command.contains("SET GAMESTATE")) {
+                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")) {
+                commandHandler.getGameSequence();
+            } else if(command.contains("EXIT")) {
+                System.out.println("Exited the Squid Game Control Room");
+                break;
+            }


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

petaripetrov · 2022-03-19T12:31:04Z

src/main/java/softwaredesign/Main.java

+    public static void main (String[] args) {
+        CommandHandler commandHandler = new CommandHandler();
+        System.out.println("Welcome to the Squid Game Control Room");
+        while (true) {


The entire while block can be formatted better, as right now it is hard to read

petaripetrov · 2022-03-19T12:33:20Z

src/main/java/softwaredesign/Main.java

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


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

petaripetrov · 2022-03-19T12:36:03Z

src/main/java/softwaredesign/Main.java

+            System.out.print("> ");
+            Scanner scanner = new Scanner(System.in);
+            String command = scanner.nextLine();
+            if(command.contains("SET GAMESTATE")) {


This can also be abstracted behind constants

petaripetrov · 2022-03-19T12:37:55Z

src/main/java/softwaredesign/CommandHandler.java

+    }
+
+    protected void setGameState(String command) {
+        String[] commandSplitted = command.split(" ");


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

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.

ielaajezdev

The code is readable and clear. I have put suggestions for a CLI class in a comment on your Main file and I also added a few command parsing suggestions.
Also: consider extending your gitignore file to also exclude .idea and .gradle folders. This will make it easier to track meaningful changes in your commits.

ielaajezdev · 2022-03-19T17:32:06Z

src/main/java/softwaredesign/Main.java

+            Scanner scanner = new Scanner(System.in);
+            String command = scanner.nextLine();
+            if(command.contains("SET GAMESTATE")) {
+                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")) {
+                commandHandler.getGameSequence();
+            } else if(command.contains("EXIT")) {


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.

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.

Case sensitive commands might not be the best choice. Consider converting strings to upper / lower case before parsing them.

ielaajezdev · 2022-03-19T17:35:18Z

src/main/java/softwaredesign/CommandHandler.java

+    public CommandHandler() {
+        games = new HashMap<>();
+        gameSequence = new ArrayList<String>();
+    }


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

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.

ielaajezdev · 2022-03-19T17:39:05Z

src/main/java/softwaredesign/CommandHandler.java

+    Map<String, String> games;
+    ArrayList<String> gameSequence;


And they can also be final.

ielaajezdev · 2022-03-19T17:40:44Z

src/main/java/softwaredesign/CommandHandler.java

+            System.out.println("Invalid command");
+            return;
+        }
+        String gameName = commandSplitted[2];


Consider using newlines between blocks of code that handle different things

altun58

Did mine a bit late so I read throught the comments of others as well.

altun58 · 2022-03-20T02:54:06Z

src/main/java/softwaredesign/CommandHandler.java

+    Map<String, String> games;
+    ArrayList<String> gameSequence;


I can see how gamesequence may be used for other things in the future hence the public class. Otherwise why not private.

altun58 · 2022-03-20T02:58:50Z

src/main/java/softwaredesign/CommandHandler.java

+    public CommandHandler() {
+        games = new HashMap<>();
+        gameSequence = new ArrayList<String>();
+    }


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.

altun58 · 2022-03-20T03:01:08Z

src/main/java/softwaredesign/CommandHandler.java

+    }
+
+    protected void setGameState(String command) {
+        String[] commandSplitted = command.split(" ");


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.

altun58 · 2022-03-20T03:03:32Z

src/main/java/softwaredesign/CommandHandler.java

+    protected void setGameState(String command) {
+        String[] commandSplitted = command.split(" ");
+        if (commandSplitted.length != 4) {
+            if(commandSplitted[0].equals("EXIT") && commandSplitted.length == 1){


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.

altun58 · 2022-03-20T03:06:40Z

src/main/java/softwaredesign/CommandHandler.java

+
+    protected void getGameState(String command) {
+        String[] commandSplitted = command.split(" ");
+        if (commandSplitted.length > 3) {


Isn't there a return at line 39? It should just reenter the loop in main.

altun58 · 2022-03-20T03:17:11Z

src/main/java/softwaredesign/Main.java

@@ -1,7 +1,27 @@
 package softwaredesign;


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.

dklbreitling

You have a lot of gradle, build and idea files that bloat your changes. If you included more of these in you .gitignore file, it would be easier to see what actually changed. See here for an example of a comprehensive .gitignore file for Java. See here for how to apply a .gitignore file to an existing repo.

dklbreitling

You use a lot of string literals directly. You could put them in constants to make sure you only have to change them in one place.

The naming of your methods, classes, and variables overall is good and follows consistent style.

dklbreitling · 2022-03-21T07:52:23Z

src/main/java/softwaredesign/CommandHandler.java

+        if (commandSplitted.length < 3) {
+            System.out.println("Invalid command");
+            return;
+        }


This is repeated code, a method to safely split the command could avoid this.

dklbreitling · 2022-03-21T07:52:40Z

src/main/java/softwaredesign/CommandHandler.java

+            System.out.println("The game " + gameName + " does not exist!");
+            return;
+        }
+        System.out.println("The gamestate of " + gameName + " is " + state);


Instead of the early return, it could have just been an if... else block.

You should actually omit as many elses as possible, so that would be a bad practice.

dklbreitling · 2022-03-21T07:54:24Z

src/main/java/softwaredesign/Main.java

+                System.out.println("Exited the Squid Game Control Room");
+                break;
+            }
+        }
    }


Make sure you close your Scanner properly.

dklbreitling · 2022-03-21T07:56:18Z

src/main/java/softwaredesign/Main.java

+    public static void main (String[] args) {
+        CommandHandler commandHandler = new CommandHandler();
+        System.out.println("Welcome to the Squid Game Control Room");
+        while (true) {


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.

dklbreitling · 2022-03-21T07:59:07Z

src/main/java/softwaredesign/Main.java

+            Scanner scanner = new Scanner(System.in);
+            String command = scanner.nextLine();
+            if(command.contains("SET GAMESTATE")) {
+                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")) {
+                commandHandler.getGameSequence();
+            } else if(command.contains("EXIT")) {


Case sensitive commands might not be the best choice. Consider converting strings to upper / lower case before parsing them.

davyvg-dev · 2022-03-22T09:55:07Z

src/main/java/softwaredesign/CommandHandler.java

+    Map<String, String> games;
+    ArrayList<String> gameSequence;


Clear naming of variables. Package and Import statements consistent with google guidelines.

davyvg-dev · 2022-03-22T09:57:34Z

src/main/java/softwaredesign/CommandHandler.java

+        String[] commandSplitted = command.split(" ");
+        if (commandSplitted.length < 3) {
+            System.out.println("Invalid command");
+            return;
+        }


As previously mentioned the code is repeated.

davyvg-dev · 2022-03-22T09:58:17Z

src/main/java/softwaredesign/CommandHandler.java

+        }
+
+        System.out.println("Set the game sequence to:");
+        for(int i = 0; i < gameSequence.toArray().length; i++) {


Can use .size() instead of converting to an array.

davyvg-dev · 2022-03-22T10:01:30Z

src/main/java/softwaredesign/Main.java

    }
-}
+}


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

somanshbudhwar · 2022-03-23T08:58:53Z

src/main/java/softwaredesign/Main.java

+    public static void main (String[] args) {
+        CommandHandler commandHandler = new CommandHandler();
+        System.out.println("Welcome to the Squid Game Control Room");
+        while (true) {


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.

somanshbudhwar · 2022-03-23T09:06:07Z

src/main/java/softwaredesign/Main.java

    }
-}
+}


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.

RDJansen531

Overall good code, only issues are the if-else block from main which should possibly be in command handler and commandSplitter being repeated.

RDJansen531 · 2022-03-23T14:31:30Z

src/main/java/softwaredesign/Main.java

    }
-}
+}


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.

RDJansen531 · 2022-03-23T14:49:21Z

src/main/java/softwaredesign/Main.java

+    public static void main (String[] args) {
+        CommandHandler commandHandler = new CommandHandler();
+        System.out.println("Welcome to the Squid Game Control Room");
+        while (true) {


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

RDJansen531 · 2022-03-23T15:01:51Z

src/main/java/softwaredesign/CommandHandler.java

+    }
+
+    protected void setGameState(String command) {
+        String[] commandSplitted = command.split(" ");


Splitting of command is repeated code, and can be handled before this function (possibly combined with the if statements in main)

RDJansen531 · 2022-03-23T15:19:11Z

src/main/java/softwaredesign/CommandHandler.java

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


Overall the functionality of the command handler looks good, especially if you include the if-else block in main inside this file.

busegundogar · 2022-03-25T19:58:30Z

src/main/java/softwaredesign/Main.java

+    public static void main (String[] args) {
+        CommandHandler commandHandler = new CommandHandler();
+        System.out.println("Welcome to the Squid Game Control Room");
+        while (true) {


I am also not sure if this part must be in main or not

busegundogar · 2022-03-25T20:01:39Z

src/main/java/softwaredesign/Main.java

    }
-}
+}


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

jaytjeboy and others added 14 commits March 9, 2022 19:32

socket conn

e585d44

netty

b357ce9

gamestate change

475b13c

exit and welcome

3748553

state checl

90705e2

Changed the check command

48571d6

Added exit function.

93bc6e2

Added jar file

ec65a6d

Added jar file

ba17cd7

Gamesequence addition

a2615ed

Jar file update

4db6ab0

Jar file update

314c5e2

Doc uploaded

832668b

Doc uploaded

ff1f197

Woutuuur reviewed Mar 19, 2022

View reviewed changes

petaripetrov reviewed Mar 19, 2022

View reviewed changes

ielaajezdev reviewed Mar 19, 2022

View reviewed changes

altun58 reviewed Mar 20, 2022

View reviewed changes

dklbreitling reviewed Mar 21, 2022

View reviewed changes

davyvg-dev self-requested a review March 22, 2022 09:47

davyvg-dev reviewed Mar 22, 2022

View reviewed changes

somanshbudhwar reviewed Mar 23, 2022

View reviewed changes

RDJansen531 self-requested a review March 23, 2022 13:01

RDJansen531 reviewed Mar 23, 2022

View reviewed changes

busegundogar reviewed Mar 25, 2022

View reviewed changes

Assignment 2 #2

Are you sure you want to change the base?

Assignment 2 #2

Conversation

Jaystah commented Mar 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ielaajezdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altun58 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dklbreitling left a comment

Choose a reason for hiding this comment

dklbreitling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RDJansen531 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment