-
Notifications
You must be signed in to change notification settings - Fork 77
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
[Bian Rui] iP #60
base: master
Are you sure you want to change the base?
[Bian Rui] iP #60
Conversation
src/main/java/Storage.java
Outdated
import java.util.Arrays; | ||
|
||
public class Storage { | ||
private static final String IDLE_MESSAGE = "Nothing happened QaQ"; |
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.
good use of constants! makes the code very neat and readable
src/main/java/Task.java
Outdated
public class Task { | ||
protected String description; | ||
protected boolean isDone; | ||
protected static int TIME_SEPARATOR_LENGTH = 5; //length of separator from input, eg: /at:, /by: |
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.
good use of constants
src/main/java/Storage.java
Outdated
*/ | ||
public void execute(String cmd) { | ||
String[] words = cmd.split(COMMAND_SEPARATOR); | ||
switch (words[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.
good switch indentations
@@ -0,0 +1,30 @@ | |||
public class Task { | |||
protected String description; | |||
protected boolean isDone; |
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.
correct naming standard of booleans
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, it is a good and neat code! All the magic literals are defined as constants. OOP, encapsulation and compartmentalization are clean! :)
src/main/java/Event.java
Outdated
|
||
@Override | ||
public String getStatusIcon() { | ||
String statusIcon = "[E]"; |
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.
Use your predefined constant EVENT_MARK
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, see comments. 👍
src/main/java/FormatChecker.java
Outdated
int TIME_IDENTIFIER_LENGTH = 5; | ||
|
||
static void checkNullString(String string) throws WrongCommandFormatException { | ||
if(string.replaceAll("\\s+","").equals("")) { |
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.
Just a space!
if(string.replaceAll("\\s+","").equals("")) { | |
if (string.replaceAll("\\s+","").equals("")) { |
src/main/java/Storage.java
Outdated
|
||
try { | ||
if (cmds[0].equalsIgnoreCase(COMMAND_MARK)) { | ||
list[Integer.parseInt(cmds[1]) - 1].setIsDone(true); |
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.
Consider breaking this up to avoid complicated expressions.
src/main/java/Todo.java
Outdated
private static final String TODO_MARK = "[T]"; | ||
|
||
/** | ||
* Deafult constructor for a Todo instance |
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.
Mini typo.
* Deafult constructor for a Todo instance | |
* Default constructor for a Todo instance |
* branch-Level-7: Add load and save file feature # Conflicts: # src/main/java/Storage.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.
Not much to add, great job 💯
|
||
@Override | ||
public String getTime() { | ||
return this.time; |
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 you can avoid the unnecessary use of this
.
return this.time; | |
return time; |
https://se-education.org/guides/conventions/java/index.html#variables
src/main/java/FileIO.java
Outdated
} | ||
|
||
static void writeFile(ArrayList<Task> Tasks) throws IOException { | ||
FileWriter fw = new FileWriter("/Users/bromine/ip/Saves/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.
Seems the Path
is repeated twice, could it be better as a constant?
update find method
No description provided.