-
Notifications
You must be signed in to change notification settings - Fork 200
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
[Gerald Koh Kheng Guan] iP #192
base: master
Are you sure you want to change the base?
Conversation
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 think your code quality is fairly decent. However, you can look at abstracting your main method using other methods or classes to make it shorter. Also you should be careful of your lack of a default statement for your switch case as it might create exceptions which your program might not be able to handle.
src/main/java/Duke.java
Outdated
public static void main(String[] args) { | ||
String logo = " ____ _ \n" | ||
+ "| _ \\ _ _| | _____ \n" | ||
+ "| | | | | | | |/ / _ \\\n" | ||
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
System.out.println("Hello from\n" + logo); | ||
Methods.printGreetings(); | ||
|
||
Task[] actions = new Task[100]; | ||
int actionCounter = 0; | ||
|
||
String line; | ||
Scanner in = new Scanner(System.in); | ||
int taskNumber; | ||
String[] decisions; | ||
Task toBeAdded; | ||
String[] dates; | ||
do { | ||
line = in.nextLine(); | ||
decisions = line.split(" "); | ||
dates = line.split("/"); | ||
switch (decisions[0]) { | ||
case "echo": | ||
System.out.println(Methods.findTaskDetails(line)); | ||
break; | ||
|
||
case "todo": | ||
toBeAdded = new Todo(Methods.findTaskDetails(line)); | ||
actions[actionCounter] = toBeAdded; | ||
actionCounter++; | ||
Methods.printAcknowledgement(toBeAdded,actionCounter); | ||
break; | ||
|
||
case "event": | ||
toBeAdded = new Event(Methods.findTaskDetails(dates[0]), Methods.findTaskDetails(dates[1]), Methods.findTaskDetails(dates[2])); | ||
actions[actionCounter] = toBeAdded; | ||
actionCounter++; | ||
Methods.printAcknowledgement(toBeAdded,actionCounter); | ||
break; | ||
|
||
case "deadline": | ||
toBeAdded = new Deadline(Methods.findTaskDetails(dates[0]), Methods.findTaskDetails(dates[1])); | ||
actions[actionCounter] = toBeAdded; | ||
actionCounter++; | ||
Methods.printAcknowledgement(toBeAdded,actionCounter); | ||
break; | ||
|
||
case "mark": | ||
taskNumber = Integer.parseInt(decisions[1]) - 1; | ||
actions[taskNumber].mark(); | ||
Methods.printDoneMarkingTasks(actions[taskNumber]); | ||
break; | ||
|
||
case "unmark": | ||
taskNumber = Integer.parseInt(decisions[1]) - 1; | ||
actions[taskNumber].unmark(); | ||
Methods.printDoneMarkingTasks(actions[taskNumber]); | ||
break; | ||
|
||
case "list": | ||
if (actionCounter == 0) { | ||
Methods.printEmptyList(); | ||
continue; | ||
} | ||
for (int iterator = 0; iterator < actionCounter; iterator++) { | ||
Methods.printListElement(iterator, actions[iterator]); | ||
} | ||
break; | ||
} | ||
} while (!decisions[0].equals("bye")); | ||
System.out.println("That's all from me! Goodbye!"); | ||
} | ||
} |
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.
Perhaps you can consider how to abstract your main method into various different methods as currently it is rather lengthy (Greater than 30 LOC).
src/main/java/Methods.java
Outdated
import java.util.Scanner; | ||
public class Methods { | ||
public static void printGreetings() { | ||
String logo = " ____ _ \n" |
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.
You might want to consider declaring these variables as constants instead as they are they do not need to change.
src/main/java/Duke.java
Outdated
Methods.printListElement(iterator, actions[iterator]); | ||
} | ||
break; | ||
} |
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.
Perhaps you could include a default branch for your switch block so in the case where your command is not caught by any of the case statements, your code will still function.
src/main/java/Methods.java
Outdated
import java.util.Scanner; | ||
public class Methods { | ||
public static void printGreetings() { | ||
String logo = " ____ _ \n" | ||
+ "| _ \\ _ _| | _____ \n" | ||
+ "| | | | | | | |/ / _ \\\n" | ||
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n" | ||
+ "__________________________\n"; | ||
System.out.println("Hello from\n" + logo); | ||
System.out.println("Hello! Do you need anything from me?\n" | ||
+ "I have only been trained to greet, echo and list you so far.\n" | ||
+ "Once my owner is more proficient in what he does, he will give me more functions!\n" | ||
+ "Key in a number based on the function\n 1)echo \n 2)todo\n 3)mark\n 4)unmark\n 5)deadline\n 6)event\n" | ||
+ "When you wish to exit, do tell me by typing : bye"); | ||
} | ||
public static void print(String input) { | ||
System.out.println(input); | ||
} | ||
|
||
public static void printEmptyList() { | ||
System.out.println("There is nothing to list!"); | ||
} | ||
|
||
public static void printListElement(int iterator, Task action) { | ||
Methods.print(iterator + 1 | ||
+ ")" | ||
+ action.toString()); | ||
} | ||
public static void printDoneMarkingTasks(Task action) { | ||
Methods.print("Done!\n" + action.toString()); | ||
} | ||
public static String findTaskDetails(String line) { | ||
return line.substring(line.indexOf(" ") + 1); | ||
} | ||
public static void printAcknowledgement(Task action, int actionCounter) { | ||
System.out.println("Got it. I've added this task:\n" + action.toString() + System.lineSeparator() + "Now you have " + actionCounter + " tasks in the list."); | ||
} | ||
} |
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 think you did a good job at making your method names simple yet descriptive. This makes it easy to follow the program flow.
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.
Good job overall on your coding standards. Remember to wrap all texts with an indent of 8 spaces.
src/main/java/Duke.java
Outdated
break; | ||
|
||
case "event": | ||
toBeAdded = new Event(Methods.findTaskDetails(dates[0]), Methods.findTaskDetails(dates[1]), Methods.findTaskDetails(dates[2])); |
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.
When wrapping texts, remember to indent 8 spaces (or 2 tabs) from former line
src/main/java/Duke.java
Outdated
Methods.printListElement(iterator, actions[iterator]); | ||
} | ||
break; | ||
} |
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.
Good job on formatting your case statements!
src/main/java/Event.java
Outdated
@Override | ||
public String toString() { | ||
return "[E]" + super.toString() + " (from: " + from + "to: " + to + ")"; | ||
|
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.
Remember to remove extra new lines in your code!
src/main/java/Methods.java
Outdated
@@ -0,0 +1,39 @@ | |||
import java.util.Scanner; | |||
public class Methods { |
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.
Good use on plurals to define your class!
src/main/java/Methods.java
Outdated
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n" | ||
+ "__________________________\n"; | ||
System.out.println("Hello from\n" + logo); |
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.
Good job on wrapping the texts!
…-> actions.size()
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.
Good job Gerald! 💯 You've mostly adhered to the coding standard and readability requirements. I have added comments wherever necessary. If you think my suggestion applies to another part of the code where I haven't commented, please make the changes accordingly.
src/main/java/DukeSession.java
Outdated
public void setUpArrayList() { | ||
FileHandler.setUpFile(actions); | ||
} | ||
public void execute() { |
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.
This method is quite long (>30 lines). Consider refactoring the code to a new method OR see how you can combine common things from the case statements together. See here
File directory = new File("data"); | ||
if (!directory.exists()) { | ||
boolean success = directory.mkdir(); | ||
if (!success) { |
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.
Avoid deep nesting to improve readability. If needed, you can refactor the inner nests to a new method. See here
File f = new File("data/savedList.txt"); | ||
File directory = new File("data"); |
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.
Perhaps avoid the usage of magic literals. You can store the string literals as constants and use them (final static vars). See here
|
||
public class FileHandler { | ||
|
||
public static void setUpFile(ArrayList<Task> actions) { |
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.
This function is also quite long, maybe see how you can refactor some things inside it.
src/main/java/utility/Methods.java
Outdated
|
||
import tasks.Task; | ||
|
||
public class Methods { |
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.
The name of this class does not describe what it is used for. Maybe consider naming itUi
instead since it seems to handle the functions for console output operations. See here
@@ -0,0 +1,114 @@ | |||
package utility; | |||
|
|||
public class commandChecker { |
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.
Class names should be in PascalCase. See here
Implemented JavaDoc and other Ui debugging and formatted strings.
Update README.md
No description provided.