-
Notifications
You must be signed in to change notification settings - Fork 454
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
[Aakansha Narain] iP #491
base: master
Are you sure you want to change the base?
[Aakansha Narain] iP #491
Conversation
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!!
Overall, your code is nice and well organized.
Just added some comment and possible improvements to make.
src/main/java/duke/EventInput.java
Outdated
int charIndex = input.indexOf("/" ); | ||
int atIndex = charIndex + 4; |
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 like how you named your variables here! (Easy to understand)
|
||
public void add (Task task) { | ||
todoList.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.
I like how you named your variables with verbs, easy to understand at one glance.
public class Event extends Task { | ||
|
||
private String at; | ||
|
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 could remove the extra empty line
src/main/java/duke/InputHandler.java
Outdated
public abstract class InputHandler { | ||
protected Ui ui; | ||
protected TaskList taskList; |
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 could add a line between? (To keep your coding style consistent, same thing with couple of other files)
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.
Hey Aakansha!
Overall, really liked the organisation of your project. It really adheres to the principles of OO! I'll be leaving some comments on some parts of your project. Once again, well done!
src/main/java/Duke.java
Outdated
@@ -1,10 +1,64 @@ | |||
//Solution below slightly adapted from https://github.com/Wincenttjoi/CS2103T-duke-chatbot/blob/master/src/main/java/duke/Duke.java | |||
|
|||
import duke.*; |
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.
Should you be including the individual packages here rather than a wildcard?
src/main/java/Duke.java
Outdated
|
||
private Ui ui; | ||
private TaskList taskList; | ||
private boolean exit; |
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 could change this to a more boolean sounding name!
src/main/java/Duke.java
Outdated
} | ||
|
||
public enum InputCommands { | ||
bye, list, done, delete, todo, deadline, event |
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 these are going to be unchanged, you could possibly rename to be capitalised? For example, BYE, LIST, DONE etc. However, i like the naming of the enum InputCommands here!
//Solution below slightly adapted from https://github.com/Wincenttjoi/CS2103T-duke-chatbot/blob/master/src/main/java/duke/Duke.java | ||
|
||
import duke.*; | ||
|
||
public class Duke { |
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 could also add some comments here! I think this would allow you to communicate your code effectively!
src/main/java/Duke.java
Outdated
bye, list, done, delete, todo, deadline, event | ||
} | ||
|
||
public void start () { |
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 you do not need to add a space after the method name here! just start() would do
src/main/java/duke/DeleteInput.java
Outdated
|
||
@Override | ||
public String handle(String input) throws EmptyDescriptionException { | ||
if (input.length() == 6) { |
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'm not too sure whether i understand the reasoning behind using length() here. Could you comment on this?
src/main/java/duke/DoneInput.java
Outdated
@Override | ||
public String handle (String input) throws EmptyDescriptionException { | ||
if (input.length() == 4) { | ||
throw new EmptyDescriptionException("error"); |
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 it's great to have different Exceptions here! really improves the OO-ness of the project.
src/main/java/duke/DoneInput.java
Outdated
throw new EmptyDescriptionException("error"); | ||
} | ||
|
||
char taskIndex = input.charAt(5); |
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 you could rename the variable here, to something like indexOfTask?
src/main/java/duke/TaskList.java
Outdated
import java.util.ArrayList; | ||
|
||
public class TaskList { | ||
private static ArrayList<Task> todoList; |
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 could rename this to tasks to keep it consistent with the coding standards provided by prof!
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.
this could also be instance level (non-static) rather than class level! I think it would be easier to keep track of the collection this way.
src/main/java/duke/TaskList.java
Outdated
return todoList; | ||
} | ||
|
||
public void add (Task 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.
again, as with other lines of code, you could remove the space between the method name and parameter parentheses
This merge includes changes to the coding standard
…A-Gradle This merges the gradle-support to implement Scenario 2 while adding Gradle to the iP
This reverts commit c228d14. Reverting commit to be made in another branch
… comments The Storage class had code at varying levels of abstraction. It had code that was responsible for parsing the stored tasks, as well as loading them into the UI at the same time. Let's move the code responsible for parsing into the Parser class to eliminate that level of abstraction from the Storage class, keeping it solely responsible for storing and loading tasks to and from the storage file. The Parser class already deals with all the relevant parsing needed to read user inputs and other text formats, so this maintains abstraction well. It also makes the code more readable as there is no convoluted parsing to read while looking at the code to load tasks to the UI.
Add assertions to some fragments of code
Modifications to the code quality
Tagging can be useful to keep track of similar tasks in the task list. Let's add a tag command word and accordingly add methods to the Parser and UI classes, as well as make changes to the toString methods to make the UI messages more readable for the user in the case they add a tag to the tasks. In keeping with the functions of the classes, the Parser parses the tag command and then the CommandHandler ensures the actual handling of the command. The UI class then returns the output message. This is in accordance to the levels of abstraction and classification established.
Add acknowledgements
Duke: Your personal task manager
Welcome to Duke! Duke provides an interface to keep track of your tasks to make sure you never fall back on work! Here's some stuff you can do with Duke:
To use Duke, all you have to do is:
An example from our previous users:
Take a look at some reviews from current users:
For more users experienced in programming, play around with the software to make it more customisable! Here's the
main
method to get you started:Get started with Duke now! 🤩