-
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
[panavdua] iP #158
base: master
Are you sure you want to change the base?
[panavdua] iP #158
Conversation
# Conflicts: # src/main/java/panav/command/DeleteCommand.java # src/main/java/panav/command/ExitCommand.java # src/main/java/panav/command/ListCommand.java # src/main/java/panav/exception/DukeException.java # src/main/java/panav/exception/InvalidInputException.java # src/main/java/panav/exception/InvalidNumberException.java # src/main/java/panav/exception/ToDoDescriptionException.java # src/main/java/panav/task/Deadline.java # src/main/java/panav/task/Event.java # src/main/java/panav/task/Task.java # src/main/java/panav/task/TaskList.java # src/main/java/panav/ui/Ui.java Resolve conflicts by basic editing
# Conflicts: # src/main/java/panav/command/DeadlineCommand.java # src/main/java/panav/command/DeleteCommand.java # src/main/java/panav/command/EditCommand.java # src/main/java/panav/command/EventCommand.java # src/main/java/panav/command/ExitCommand.java # src/main/java/panav/command/ListCommand.java # src/main/java/panav/command/TodoCommand.java Resolve by basic editing
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.
Overall, really well done!
String keyWord; | ||
|
||
public FindCommand(String keyWord) { | ||
this.keyWord = keyWord; |
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 think keyWord can be written as keyword instead.
private String todoMessage; | ||
|
||
public TodoCommand(String todoMessage) { | ||
this.todoMessage = todoMessage; |
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.
Could be toDoMessage because of camelcase.
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.
Overall, a very great job Panav :D I hope to write code as well as you do!
public class TodoCommand extends Command { | ||
|
||
public static final String COMMAND_WORD = "todo"; | ||
private String todoMessage; |
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 toDoMessage may be better?
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.
Yup agreed, thanks for the input!
if (indexBy != -1) { | ||
textToAdd += String.format("%c ~ %d ~ %s ~ %s %n", str.charAt(1), check, | ||
str.substring(7, indexBy - 2), str.substring(indexBy + 4, length - 1)); | ||
} else if (indexFrom != -1) { | ||
textToAdd += String.format("%c ~ %d ~ %s ~ %s ~ %s %n", str.charAt(1), check, | ||
str.substring(7, indexFrom - 1), str.substring(indexFrom + 6, indexTo - 1), | ||
str.substring(indexTo + 4, length - 1)); | ||
} else { | ||
textToAdd += String.format("%c ~ %d ~ %s %n", str.charAt(1), check, str.substring(7)); | ||
} |
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.
Great code quality that you have shown, using if, else if, and else 馃憤
tasks = new ArrayList<>(); | ||
for (Task task : src) { | ||
tasks.add(task); | ||
} |
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.
Great code quality that you have shown, as the add function in ArrayList is very efficient! :D
*/ | ||
public ToDo(String description) throws ToDoDescriptionException { | ||
super(description); | ||
if (description.length() == 0) { |
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 !description.length() is better?
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.
True, shall make the change
* @return "X" is task is done, " " otherwise. | ||
*/ | ||
public String getStatusIcon() { | ||
return (isDone ? "X" : " "); // mark done task with X |
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.
Great code quality shown! The choice of conditionals over if-else statements make your code looks cleaner!
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.
LGTM, just some minor suggestions to improve code quality 馃槃
* @throws InvalidNumberException If the index doesn't exist. | ||
*/ | ||
public static int readNumber(String command, int len) throws InvalidNumberException { | ||
int number = Integer.parseInt(String.valueOf(command.charAt(command.length() - 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.
Perhaps can split this up into variables instead of nested parentheses to improve code quality
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.
Agreed, thanks for the inputs!
src/main/java/panav/Panav.java
Outdated
if (c.toString().compareTo(ListCommand.COMMAND_WORD) != 0 | ||
|| c.toString().compareTo(ExitCommand.COMMAND_WORD) != 0) { |
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.
Maybe can split this into more easily understandable variables
* @throws InvalidNumberException If the index doesn't exist. | ||
*/ | ||
public static int readNumber(String command, int len) throws InvalidNumberException { | ||
int number = Integer.parseInt(String.valueOf(command.charAt(command.length() - 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.
Can reduce this to more readable variables too
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.
Thanks for pointing it out :)
Add assertions to highlight important assumptions in the code
# Conflicts: # src/main/java/panav/MainWindow.java
Branch a code quality
User should have the ability to find all the todos/deadlines/events in the task list. Let's add functionality so the user can just type "find todo" to list out all the todos in the list. Makes it easier for users to filter out tasks of one type if the list is too long.
'searchable' part of a string representation of a task
marked; same for unmark as well
Panav ChatBot
Panav frees your mind of having to remember things you need to do and doubles your productivity. It's,
All you need to do is,
The best part is that it is completely FREE
Features:
If you are a Java programmer, you can use it to practice Java too! Here's the
main
method: