-
Notifications
You must be signed in to change notification settings - Fork 462
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
[Francis Tan] iP #522
base: master
Are you sure you want to change the base?
[Francis Tan] iP #522
Conversation
…ead of reading the input itself, now read words from the input split by " ", to make code more readable
…ags/datetimes, invalid args for un/mark
…nds, along with more javadocs
…tasks and services
…ifted static methods over to them
…n optional (h:mm)am/pm, and the description shows the month in english
…nder new timing format
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.
Very nice! Code is clean and readable, good amount of test cases to cover many bases.
src/main/java/duke/services/UI.java
Outdated
package duke.services; | ||
|
||
/** Handles display */ | ||
public class UI { |
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 the name of this class would be better written as Ui, to conform to the coding standard?
if (words.length == 1 && words[0].equals("list")) { | ||
TaskList.listTasks(); //could put words.length == 1 cases all here | ||
} else if (words.length == 1 && words[0].equals("SAVE")) { | ||
Storage.wipeDataOnExit(false); | ||
} else if (words.length == 1 && words[0].equals("WIPE")) { | ||
Storage.wipeDataOnExit(true); | ||
} else if (words[0].equals("todo")) { | ||
TaskList.addTodo(words); | ||
} else if (words[0].equals("deadline")) { | ||
TaskList.addDeadline(words); | ||
} else if (words[0].equals("event")) { | ||
TaskList.addEvent(words); | ||
} else if (words[0].equals("mark")) { | ||
TaskList.markTaskAsDone(words); | ||
} else if (words[0].equals("unmark")) { | ||
TaskList.markTaskAsNotDone(words); | ||
} else if (words[0].equals("delete")) { | ||
TaskList.deleteTask(words); | ||
} else if (words[0].equals("find")) { | ||
TaskList.findTasksContainingKeyword(words); | ||
} else { | ||
UI.sayLines(new String[]{"I'm sorry, I don't know what that means"}); | ||
} |
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 a switch statement would be more conventional for readibility here?
*/ | ||
public static void handleUserInputs() { | ||
Scanner inputScanner = new Scanner(System.in); | ||
String[] words = Arrays.stream(inputScanner.nextLine().strip().split(" ")).toArray(String[]::new); |
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.
Nice application of declarative input consumption and splitting here! :)
assertEquals(Parser.getDescription(deadline1, "/by"), "finish essay"); | ||
assertEquals(Parser.getTiming(deadline1, "/by"), "tmr night or ask 4 extension"); | ||
//does it throw exception for missing all other arguments? | ||
assertThrows(IllegalArgumentException.class, () -> |
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 like how you also decided to test the exceptions here!
But this test case may be testing too many things at the same time, perhaps split these tests into more subgroups? Like
testGetDescription_event
, testGetDescription_deadline
, testGetDescriptionAndDate_throwsIllegalArgumentException
, etc
Refactored code by making Duke, rather than the parser, decide how to respond to inputs, and merged UI with Duke, since UI was in charge of too little functionality that Duke could also handle reasonably. Broke down a very long test case into smaller test cases.
Improve code quality
# Conflicts: # src/main/java/duke/services/Duke.java # src/main/java/duke/services/Parser.java # src/main/java/duke/services/Storage.java
and number of time attributes
Duke
Duke frees your mind of having to remember things you need to do. It's
FastSUPER FAST to useAll you need to do is
And it is FREE!
Features:
If you're a Java programmer, you can use it to practice Java too. Here's the main method: