-
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Yao Jiaxin] iP #198
base: master
Are you sure you want to change the base?
[Yao Jiaxin] iP #198
Conversation
…asses into different files.
…he format YYYY-MM-DD for Deadline.
# Conflicts: # src/main/java/Duke.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.
The code is easy to read and understand! Well done! Just there are a few minor coding quality violations.
case "bye": | ||
return true; | ||
case "list": | ||
System.out.println("Here are the tasks in your list my premier:"); | ||
this.tasks.printList(); | ||
break; | ||
default: | ||
String[] arrOfStr = command.split(" ", 2); | ||
try { | ||
validateCmd(arrOfStr[1]); | ||
} catch (MissingDescriptionException e) { | ||
System.out.println(e.getMessage()); | ||
} |
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.
Will it be better if to add indentation after switch?
* @return String representation of Task. | ||
*/ | ||
public String toString() { | ||
String var10000 = this.getStatusIcon(); |
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 varible name 'var10000' be changed so reader can better understand?
src/main/java/duke/Duke.java
Outdated
this.ui.sayBye(); | ||
} | ||
storage.saveToFile(tasks); | ||
//... |
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 the dead code shoule be deleted?
* A storage instance handles all interactions between local data file and Duke. | ||
*/ | ||
public class Storage { | ||
private File myFoo; |
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 using od 'myFile' instead of 'myFoo' is better?
public void testToStringComplete() { | ||
Todo task = new Todo("This is Todo."); | ||
task.changeCompletion(); | ||
String actualOutput = task.toString(); | ||
assertEquals("[T][X] This is Todo.", actualOutput); |
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 use the functions. Good job!
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 your code looks great! I just have a few minor suggestions.
*/ | ||
public void saveToFile(TaskList itemList) { | ||
String path = "data/duke.txt"; | ||
FileWriter fooWriter = null; //init |
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.
Building on from before, maybe this can also become a fileWriter instead of a fooWriter?
public TaskList() { | ||
this.arrList = new ArrayList<Task>(); | ||
} | ||
public int size() { |
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 be changed to getSize() so that the method name is a verb?
|
||
import java.util.ArrayList; | ||
|
||
public class 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.
Remember to add comments to the class and methods in this file!
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.
Sorry, just added a few extra comments.
} catch (MissingDescriptionException e) { | ||
System.out.println(e.getMessage()); | ||
} | ||
switch (arrOfStr[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.
Your switch is really neatly organised!
this.arrList.set(index-1, toMark); | ||
} | ||
public void setToUnmark(int index) { | ||
Task toMark = arrList.get(index - 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.
Maybe this can be changed into a better noun? I kind of thought that with the existence of verb methods like toString, toMark would also be considered as a verb, but I can also see that toMark can serve as a noun, so maybe keeping it as it is is good.
There are currently no assert statements in the code to test for correctness of the program during execution. Adding asserts help to test the program correctness and avoid bugs on runtime. As a step forward, we can add more asserts in future feature implementations.
Parser method was too length and violated SLAP. Abstracting away the details into new classes help to make code more readable. Future implementations follow SLAP principle to make the code readable and easy to build on.
Refactor code to align with coding quality
Branch a assertions
Lack support for a task period to be done within a time frame. Creating this adds more functionality. Can implement more relevant functions such as do after a certain time period.
Future implementation can include dynamic resizing of the window.
Duke Atreides
Fearing all your deadlines? Or rather, do you fear forgetting these deadlines? Then fear not, for Duke Atreides of Caladan is here! It can help you look remember your tasks so you do not need to fear forgetting them again, because Duke Atreides is
evil like the HarkonnensSUPER FAST to useAll you need to do is to
And it is FREE!
Features:
Here is the main method for those back in Caladan