-
Notifications
You must be signed in to change notification settings - Fork 364
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
[szejiancheng] iP #205
base: master
Are you sure you want to change the base?
[szejiancheng] iP #205
Conversation
…es to reflect new error messages in EXPECTED.txt and input.txt
From Level-7 requirements. Using java.nio.file classes to save on String parsing.
In order to facillitate more complicated instruction parsing, consolidated all command parsing under recieveInput
Taught duke how to understand dates
# Conflicts: # src/main/java/DukeBehaviour.java
What it says on the tin, was confused and had to figure out where Level-7 went to
Encapsulate the various components of Duke's execution in seperate classes
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.
Looks good, just some small cosmetic changes. The code was easy to read and made sense!
src/main/java/Parser.java
Outdated
ArrayList<String> tokens = new ArrayList<>(Arrays.asList(input.split(" "))); | ||
tokens.removeIf(s -> s.equals(" ") || s.equals("")); | ||
tokens.forEach(s -> s = s.trim()); | ||
//System.out.println(tokens); |
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.
Can remove this.
src/main/java/Task.java
Outdated
this.isDone = false; | ||
} | ||
|
||
|
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.
Can remove.
super(tokens); | ||
int byId = tokens.indexOf("/by"); | ||
if (byId < 0){ | ||
throw new DukeException("Invalid input received! \nDeadline commands are in the form of: deadline name /by bytime \n(remember to include '/by')"); |
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.
Can add line breaks for style.
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.
Looks good to me. Just some minor styling issues here and there.
String name = String.join(" ", tokens.subList(1, byId)); | ||
LocalDate by = LocalDate.parse(String.join(" ", tokens.subList(byId+1, tokens.size()))); | ||
Deadline newDeadline = new Deadline(name, by); | ||
super.setTaskToAdd(newDeadline); |
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.
As AddDeadlineCommand extends AddCommand, the command setTaskToAdd should work without the super in front. You should be able to just call it directly.
src/main/java/Deadline.java
Outdated
|
||
@Override | ||
public String toString() { | ||
return "[D]" + super.toString() + " (by: " + by.format(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.
As [D] shows up in multiple places, you may want to consider making it a constant within the class
src/main/java/DeleteCommand.java
Outdated
static final int commandLength = 2; | ||
static final int indexOfIndex = 1; |
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.
As these 2 variables are final, should consider probably fully capitalising them like COMMAND_LENGTH for example.
src/main/java/DeleteCommand.java
Outdated
static final int commandLength = 2; | ||
static final int indexOfIndex = 1; | ||
|
||
private Task taskToDeleted; |
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.
taskToDeleted sounds grammatically incorrect. Maybe you meant taskToDelete or taskToBeDeleted.
src/main/java/Event.java
Outdated
protected LocalDate from; | ||
static DateTimeFormatter format = DateTimeFormatter.ofPattern("MMM d yyyy"); | ||
protected LocalDate 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.
you may want to consider rearranging the order, perhaps static variables together and non static ones together.
src/main/java/Event.java
Outdated
|
||
@Override | ||
public String toString() { | ||
return "[E]" + super.toString() + " (from: " + from.format(format) + " to: " + to.format(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.
Same thing, you might want to abstract out the status icon
src/main/java/MarkCommand.java
Outdated
int index = Integer.parseInt(tokens.get(1)); | ||
markIndex = index; |
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.
Why not just directly assign?
src/main/java/Parser.java
Outdated
} | ||
private static ArrayList<String> tokenize(String input) { |
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.
Since you have been putting a newline between every function so far, might be good to keep consistent and put newline between functions here as well.
src/main/java/Parser.java
Outdated
switch (key) { | ||
case "bye": |
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.
According to the java styles, the switch and case should be in the same indentation.
src/main/java/Task.java
Outdated
this.isDone = false; | ||
} | ||
|
||
} |
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.
May want to add new line to the end of your file
Added FindCommand.java, additional switch line in Parser.java, print result function in Ui
# Conflicts: # src/main/java/duke/components/Storage.java
Also refactored code to make more sense now that we've removed the Ui class.
Code does not fully follow the specified code style, negatively impacting its readability Amending all checkstyle violations improves the code quality. In order to improve code quality, lets resolve all checkstyle violations. This will improve readability as well as ease of documentation.
Fix checkstyle violations
Add assertions to component classes
Previously the application did not exit properly when the bye command was invoked, which was not expected behaviour By invoking System.exit(), the command behaves as expected
If a user wants to directly see what tasks they have upcoming, there is no current functionality to sort tasks by date, or completion status. By adding a remind command, the user is able to get a glance of their upcoming uncompleted tasks within a timeframe they desire (in days) In our push for optimal user experience, lets add this functionality.
Introducing Duke, the CLI task assistant!
Duke stores all your tasks so you don't have to! Now supporting:All you need to do is,
Features: