-
Notifications
You must be signed in to change notification settings - Fork 437
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
[Lin Yuan Xun, Caleb] iP #102
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # src/main/java/Bob.java Resolution: keep all changes
Improve coding standard
# Conflicts: # src/main/java/AddCommand.java # src/main/java/Bob.java # src/main/java/BobIncompleteEventDescriptionException.java # src/main/java/Deadline.java # src/main/java/DeleteCommand.java # src/main/java/Event.java # src/main/java/ExitCommand.java # src/main/java/Storage.java # src/main/java/Task.java # src/main/java/TaskList.java # src/main/java/Todo.java # src/main/java/UI.java Resolution: Keep javadocs and coding standard improvements
# Conflicts: # data/save.txt # src/main/java/TaskList.java Resolution: keep all changes made in the two branches.
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.
The main functionality looks good!
Some parts of the code, such as the Parser class, could do with more documentation as I was rather confused as to what the code is doing.
A few cosmetic changes requested.
data/save.txt
Outdated
@@ -0,0 +1,10 @@ | |||
<<<<<<< HEAD |
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 might have been missed while merging! Should this be here?
src/main/java/Bob.java
Outdated
@@ -0,0 +1,41 @@ | |||
import main.java.*; |
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.
Would it be clearer to list the imported classes explicitly?
src/main/java/Bob.java
Outdated
public class Bob { | ||
private TaskList tasks = new TaskList(); | ||
private Storage storage; | ||
private UI uI; |
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 this variable be named "ui" instead?
/** | ||
* An exception that may be thrown when an error occurs during the parsing of dates and times. | ||
*/ | ||
public class BobDateTimeParseException extends BobException { |
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 that you made classes for the specific types of exceptions!
src/main/java/Event.java
Outdated
*/ | ||
public Event (String description, String period) { | ||
super(description); | ||
this.start = LocalDateTime.parse(period.substring(0,15), inputFormatter); |
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 the call to .substring be formatted in a better way? Noticed this in a few other places as well!
src/main/java/ExitCommand.java
Outdated
/** | ||
* Returns false to indicate that Bob may terminate. | ||
* | ||
* @return false. |
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 this method be returning false? It's currently returning true.
src/main/java/Storage.java
Outdated
} | ||
} | ||
|
||
/**Updates the Storage's text file according to the tasks in a provided 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.
Should this JavaDoc be in the next line?
src/main/java/Storage.java
Outdated
|
||
/** | ||
* Closes the Storage's writer. | ||
* @throws BobIOException if the Storage's text file does not exist. |
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 this line be 1 extra line below the previous? Noticed this in some other places too!
src/main/java/Todo.java
Outdated
* @param isDone indicates if the todo is completed. | ||
* @param description brief description of the todo. | ||
*/ | ||
|
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.
Noticed some extra lines after JavaDocs. Should the JavaDoc style be consistent?
src/main/java/UI.java
Outdated
* @throws BobIndexOutOfBoundsException if the method tries to retrieve a task with an index not on the list. | ||
*/ | ||
public void printOutList(TaskList tasks) throws BobIndexOutOfBoundsException { | ||
for(int i = 1; i < tasks.size()+1; i++) { |
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 this line be better spaced?
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.
The coding style looks good!
src/main/java/AddCommand.java
Outdated
* @return true or false if AddCommand is equal or not equal to the object respectively. | ||
*/ | ||
@Override | ||
public boolean equals(Object o) { |
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 the variables for be more descriptive? That is, instead of o, be obj, and instead of c, be cmd
src/main/java/Task.java
Outdated
*/ | ||
public class Task { | ||
/** A brief description of the task. */ | ||
protected String description; |
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 they have been private instead of protected?
src/main/java/TaskList.java
Outdated
public class TaskList { | ||
|
||
/** An array list consisting of tasks. */ | ||
private ArrayList<Task> list; |
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 this have been renamed as tasks?
src/main/java/UI.java
Outdated
public void printOutList(TaskList tasks) throws BobIndexOutOfBoundsException { | ||
for(int i = 1; i < tasks.size()+1; i++) { | ||
Task task = tasks.get(i); | ||
System.out.println(i +"." + task.toString()); |
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 there be spacing between +"." ?
src/main/java/UI.java
Outdated
Task task = tasks.get(i); | ||
System.out.println(i +"." + task.toString()); | ||
} | ||
if(tasks.isEmpty()) { |
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 there be spacing between if(..)?
add assertions to code
# Conflicts: # src/main/java/bob/command/DeleteCommand.java Resolution: Retain changes made by both branches
Change Bob profile picture Improve label design and font Improve error message GUI
-Change exception messages -Edit exception handling of Parse.parseDelete and Parse.parseDone
-Add option to delete all tasks -Add option to mark all tasks as done
-Add javadocs for new methods. -Edit old javadocs
No description provided.