[Tan Gee Teng] iP #281
Open
[Tan Gee Teng] iP #281
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
yunbinmo
reviewed
Feb 1, 2022
seksek13
reviewed
Feb 4, 2022
| @@ -85,4 +85,25 @@ public String count() { | |||
| return "\n Now you have " + tasks.size() + " task" + isSingular + " in your list."; | |||
| } | |||
|
|
|||
| public String find(String keyword) { | |||
| String resultStr = ""; | |||
| boolean keywordFound = false; | |||
There was a problem hiding this comment.
Boolean variable should use a prefix such as is, has, was.
src/main/java/seedu/duke/Ui.java
Outdated
| @@ -40,6 +44,9 @@ public void nextInput(String input, TaskList tasks) { | |||
| case DELETE: | |||
| myPrint(tasks.delete(Parser.getIndex(input))); | |||
| break; | |||
| case FIND: | |||
| myPrint(tasks.find(Parser.getDescription(input))); | |||
There was a problem hiding this comment.
Maybe it will be better to change the myprint() method name to something that would explain the function better
| * @return String that describes the number of tasks in the TaskList | ||
| */ | ||
| public String count() { | ||
| String isSingular = "s"; |
There was a problem hiding this comment.
Why does the string variable had the prefix "is", it might be confusing for others as this prefix is reserved for boolean variables. It might be better to rename this variable :)
| tasks.add(new Event("party ", LocalDate.of(2022, 12, 12))); | ||
| tasks.add(new ToDo("borrow book")); | ||
|
|
||
| TaskList t = new TaskList(tasks); |
There was a problem hiding this comment.
is it possible to rename this variable name so that it would be clearer in showing what you are doing?
Code was messy and might be unreadable Code quality improvement helps for future reference and checking. Let's * check for error-prone practices * delete all dead code
Find command is case sensitive A case insensitive find is more user-friendly because users cannot be expected to remember the exact case of the keywords. Let's update the search algorithm to use case-insensitive matching
Branch merging pr
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.
DukePro
DukePro 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 Java programmer, you can use it to practice Java too. Here's the
mainmethod: