[cetigerlily] iP #89
Open
[cetigerlily] iP #89
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
Bransonlj
reviewed
Feb 2, 2023
src/main/java/duke/tasks/Task.java
Outdated
|
|
||
| public class Task { | ||
| private String description; | ||
| private boolean status; |
There was a problem hiding this comment.
maybe "status" could be renamed to sound more booleanish like isDone or isComplete!
| import duke.tasks.Task; | ||
|
|
||
| public class DeleteCommand implements Command { | ||
| private String input; |
There was a problem hiding this comment.
maybe can add spaces between these lines!
| @@ -0,0 +1,7 @@ | |||
| package duke.exceptions; | |||
|
|
|||
| public class DukeExceptions extends Exception { | |||
There was a problem hiding this comment.
Maybe can add javadoc for this and other classes to explain what it is for?
Comment on lines
+48
to
+50
| timeFrame = eventStart.format(DateTimeFormatter.ofLocalizedDate(FormatStyle.SHORT)) + " " + | ||
| eventStart.format(DateTimeFormatter.ofPattern("HH:mm")) + " to: " + | ||
| eventEnd.format(DateTimeFormatter.ofPattern("HH:mm")) + ")"; |
There was a problem hiding this comment.
Would it be better to have like breaks before the operator? I noticed this violation of coding standards in other places too.
src/main/java/tigerlily/Parser.java
Outdated
Comment on lines
37
to
65
| public Command handleCommand(String input) throws DukeExceptions { | ||
| try { | ||
| CommandType command = CommandType.valueOf(input.split(" ")[0].toUpperCase()); | ||
| switch (command) { | ||
| case BYE: | ||
| return new ByeCommand(); | ||
| case LIST: | ||
| return new ListCommand(); | ||
| case TODO: | ||
| return new ToDoCommand(input); | ||
| case DEADLINE: | ||
| return new DeadlineCommand(input); | ||
| case EVENT: | ||
| return new EventCommand(input); | ||
| case MARK: | ||
| return new MarkCommand(input); | ||
| case UNMARK: | ||
| return new UnmarkCommand(input); | ||
| case DELETE: | ||
| return new DeleteCommand(input); | ||
| case FIND: | ||
| return new FindCommand(input); | ||
| default: | ||
| throw new UnknownCommandException(); | ||
| } | ||
| } catch (IllegalArgumentException e) { | ||
| throw new UnknownCommandException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I like the use of a switch rather than multiple if/nested-if statements for ur handleCommand method.
Comment on lines
1
to
10
| package tigerlily.commands; | ||
|
|
||
| import tigerlily.Storage; | ||
| import tigerlily.Ui; | ||
| import tigerlily.exceptions.DukeExceptions; | ||
| import tigerlily.tasks.TaskList; | ||
|
|
||
| public interface Command { | ||
| void execute(TaskList taskList, Ui ui, Storage storage) throws DukeExceptions; | ||
| } |
There was a problem hiding this comment.
I like how a separate Command class is used to handle all the commands.
Comment on lines
+61
to
+62
| FileWriter fw = new FileWriter(this.filePath); | ||
| StringBuilder sb = new StringBuilder(); |
There was a problem hiding this comment.
Is using more meaningful variable names a better option here?
Assertions are not present in code. Assertions are useful because they allow for the testing for validity of assumptions which were made in the program. Let’s add assertions to Parser, TaskList, Mark Command, and Unmark Command. Having assertions helps the smooth running of our code.
Code can be further reviewed to adhere to proper code quality standards. Code quality is important because it not only allows other users to read and understand it better, it will help you understand your own code better too. Let's edit the existing code to follow proper code quality standards. Having clean code allows us to understand our code better.
Add assertions
Review code quality
# Conflicts: # docs/Ui.png
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.
🌷 tigerlily to-do 🌷
tigerlily to-do...
in it's current version, tigerlily to-do offers:
want to get tigerlily to-do? simply follow these steps:
if you're also a java programmer, tigerlily to-do can be used to practice java