[butteredyakiimo] iP #525
Open
[butteredyakiimo] iP #525
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZD292
reviewed
Sep 5, 2023
| @@ -15,3 +15,5 @@ bin/ | |||
|
|
|||
| /text-ui-test/ACTUAL.TXT | |||
| text-ui-test/EXPECTED-UNIX.TXT | |||
|
|
|||
| /src/main/java/*.class | |||
antonTan96
reviewed
Sep 7, 2023
src/main/java/duke/Duke.java
Outdated
Comment on lines
38
to
42
| if (parser.isGoodbye(input)) { | ||
| break; | ||
| } else { | ||
| parser.parseInput(input); | ||
| } |
There was a problem hiding this comment.
The way you check for a terminating condition is quite intuitive by checking whether the input is "bye" or not. However, this involves throwing the input into the parser twice. Would it be better to let the parser handle the case "bye" in parser.parseInput(input) ?
src/main/java/duke/Parser.java
Outdated
Comment on lines
37
to
39
| private static boolean isNumber(String s) { | ||
| return s != null && s.matches("[0-9.]+"); | ||
| } |
src/main/java/duke/Parser.java
Outdated
Comment on lines
142
to
154
| if (inputArr.length == 1) { | ||
| throw new NoTaskIdException(); | ||
| } else { | ||
| String strIndex = inputArr[1]; | ||
| if (isNumber(strIndex)) { | ||
| int index = Integer.parseInt(strIndex) - 1; //because index starts from 1 | ||
| if (tasks.isValidTaskId(index)) { | ||
| tasks.markTask(index); | ||
| } | ||
| } else { | ||
| throw new InvalidTaskIdException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
What happens if tasks.isValidTaskId(index) returns false? Could InvalidTaskIdException be thrown? I would write this part like this:
if (inputArr.length == 1) {
throw new NoTaskIdException();
} else {
String strIndex = inputArr[1];
if (isNumber(strIndex) && tasks.isValidTaskId(Integer.parseInt(strIndex) - 1)) {
int index = Integer.parseInt(strIndex) - 1; //because index starts from 1
tasks.markTask(index);
} else {
throw new InvalidTaskIdException();
}
}
src/main/java/duke/Parser.java
Outdated
Comment on lines
168
to
180
| if (inputs.length == 1) { | ||
| throw new NoTaskIdException(); | ||
| } else { | ||
| String strIndex = inputs[1]; | ||
| if (isNumber(strIndex)) { | ||
| int index = Integer.parseInt(strIndex) - 1; //because index starts from 1 | ||
| if (tasks.isValidTaskId(index)) { | ||
| tasks.unMarkTask(index); | ||
| } | ||
| } else { | ||
| throw new InvalidTaskIdException(); | ||
| } | ||
| } |
src/main/java/duke/Parser.java
Outdated
Comment on lines
216
to
240
| if (inputs.length == 1) { | ||
| throw new NoDescException(); | ||
| } else { | ||
| String afterCommand = inputs[1]; | ||
| String[] details = afterCommand.split(" /by ", 2); | ||
|
|
||
| if (details.length < 2) { | ||
| throw new InvalidDeadlineException(); | ||
| } else { | ||
| String desc = details[0]; | ||
| String date = details[1]; | ||
|
|
||
| if (desc.isEmpty()) { | ||
| throw new NoDescException(); | ||
| } else { | ||
| try { | ||
| LocalDateTime dateTime = convertToDateTime(date); | ||
| Deadline deadline = new Deadline(0, desc, dateTime); | ||
| tasks.addTask(deadline); | ||
| } catch (DateTimeParseException e) { | ||
| ui.showInvalidDateFormat(); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This part is quite heavily nested. Could you try to refactor the code to make it more flatter?
src/main/java/duke/Parser.java
Outdated
Comment on lines
255
to
300
| public void parseEvent(String input) throws NoDescException, NoStartException, NoEndException, | ||
| InvalidStartEndException, InvalidEventException { | ||
| String[] inputs = input.split(" ", 2); | ||
| if (inputs.length == 1) { | ||
| throw new NoDescException(); | ||
| } else if (inputs[1].isBlank()) { | ||
| throw new NoDescException(); | ||
| } else { | ||
| String afterCommand = inputs[1]; | ||
| String[] details = afterCommand.split(" /from ", 2); | ||
|
|
||
| if (details[0].isBlank()) { | ||
| throw new NoDescException(); | ||
| } else if (details.length == 1) { | ||
| throw new InvalidEventException(); | ||
| } else { | ||
| String task = details[0]; | ||
| String start = details[1].split(" /to ", 2)[0]; | ||
|
|
||
| if (start.isBlank()) { | ||
| throw new NoStartException(); | ||
| } else { | ||
| String[] endDetails = afterCommand.split(" /to ", 2); | ||
|
|
||
| if (endDetails.length == 1) { //no end date added | ||
| throw new NoEndException(); | ||
| } else { | ||
| String end = endDetails[1]; | ||
|
|
||
| if (end.isBlank()) { | ||
| throw new NoEndException(); | ||
| } else { | ||
| try { | ||
| LocalDateTime startDateTime = convertToDateTime(start); | ||
| LocalDateTime endDateTime = convertToDateTime(end); | ||
| Event event = new Event(0, task, startDateTime, endDateTime); | ||
| tasks.addTask(event); | ||
| } catch (DateTimeParseException e) { | ||
| ui.showInvalidDateFormat(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
src/main/java/duke/Storage.java
Outdated
| dataFile = createDataFile(); | ||
| } | ||
|
|
||
| //create a FileWriter object to write to file. Note that this overwrites the existing data! |
There was a problem hiding this comment.
I like this warning for i have fallen for this so many times D:
I still don't know how FileWriters work
Assertions help to detect possible bugs, as they verify certain assumptions that the code makes. Gradle run is also configured to enable assertions.
Some methods are extremely long, and have more than one level of abstraction. Magic numbers are used to describe the task status. Let's make more methods to adhere to SLAP and avoid nested code, and also create a new Enum to make code easier to understand.
Add assertions to some classes
Modify code to adhere to code quality standards
Currently, users only have a list commmand to list all the tasks. To create reminders for upcoming tasks, users need to be able to view the tasks that are due before/within the time period. A flag can be used at the end of the list command of the format: 'list flag'. The time period for both deadline and event starts at the start of the day at 00:00. The end is determined by the flag indicated by the user. Let's add the following flags: * 'today' : lists all tasks that in the period today 00:00 to tmr 00:00 * 'week' : lists all tasks that are in the period today 00:00 to one week later 00:00 * 'fort': lists all tasks that are in the period today 00:00 to 2 weeks later 00:00
Currently, users only have a list commmand to list all the tasks. To create reminders for upcoming tasks, users need to be able to view the tasks that are due before/within the time period. A flag can be used at the end of the list command of the format: 'list flag'. The time period for both deadline and event starts at the start of the day at 00:00. The end is determined by the flag indicated by the user. Let's add the following flags: * 'today' : lists all tasks that in the period today 00:00 to tmr 00:00 * 'week' : lists all tasks that are in the period today 00:00 to one week later 00:00 * 'fort': lists all tasks that are in the period today 00:00 to 2 weeks later 00:00
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BUTTER
BUTTER helps you remember what you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features:
If you are a Java programmer, you can use it to practise Java too. Here's the
mainmethod: