-
Notifications
You must be signed in to change notification settings - Fork 270
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[Sim Jia Ming] iP #316
base: master
Are you sure you want to change the base?
[Sim Jia Ming] iP #316
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.
Good job overall, I did not notice any basic styling errors. You can improve your code by breaking down long parts into separate functions. All the best!
src/main/java/Deadline.java
Outdated
} | ||
|
||
@Override | ||
String getSym() { |
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 would opt for a more intuitive name rather than sym.
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 would opt for a more intuitive name rather than sym.
Maybe you can change it to a more descriptive name such as symbol?
src/main/java/Duke.java
Outdated
String phrase = sc.nextLine(); | ||
System.out.println(DASH); | ||
|
||
if (phrase.equals("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.
Perhaps you can consider using a switch statement instead?
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 here is pretty long, but you will get to break it down later as you progress with the iP. Consider doing the printing in separate functions and get your switch statement to invoke those print functions instead.
src/main/java/Duke.java
Outdated
String noOfTask = String.format("Now you have %d tasks in the list.", arrlst.size()); | ||
System.out.println(noOfTask); | ||
} | ||
} catch (ArrayIndexOutOfBoundsException aioobe) { // echo |
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 can consider changing the parameter name to just e
instead.
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 nits to fix
src/main/java/Task.java
Outdated
@@ -0,0 +1,45 @@ | |||
public class 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.
It is possible to change Task to an abstract class?
src/main/java/Action.java
Outdated
@@ -0,0 +1,35 @@ | |||
import java.util.ArrayList; | |||
|
|||
public class Action { |
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.
Looks clean!
src/main/java/Deadline.java
Outdated
} | ||
|
||
@Override | ||
String getSym() { |
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 would opt for a more intuitive name rather than sym.
Maybe you can change it to a more descriptive name such as symbol?
src/main/java/Duke.java
Outdated
System.out.println("Hello from\n" + logo); | ||
Scanner sc = new Scanner(System.in); | ||
ArrayList<Task> arrlst = new ArrayList<>(); | ||
String DASH = "____________________________________________________________"; |
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 works too, but UI can be separated into a class for cleanliess
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.
Generally clean code, well documented, styled, and easy to understand. Only minor nitpicks in terms of documentation and code logic here and there. Well done!
src/main/java/Task.java
Outdated
public Task(String description) { | ||
this.description = description; | ||
this.isDone = false; | ||
this.sym = " "; | ||
} | ||
|
||
public Task(String description, String sym) { | ||
this.description = description; | ||
this.isDone = false; | ||
this.sym = sym; | ||
} | ||
|
||
public String getStatusIcon() { | ||
return (isDone ? "X" : " "); // mark done task with X | ||
} | ||
|
||
public void markAsDone() { | ||
this.isDone = true; | ||
System.out.println("Nice! I've marked this task as done:"); | ||
String output = String.format(" [%s][%s] %s", this.sym, this.getStatusIcon(), this.description); | ||
System.out.println(output); | ||
} | ||
|
||
public void markAsNotDone() { | ||
this.isDone = false; | ||
System.out.println("OK, I've marked this task as not done yet:"); | ||
String output = String.format(" [%s] %s", this.getStatusIcon(), this.description); | ||
System.out.println(output); | ||
} | ||
|
||
String getSym() { | ||
return this.sym; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return String.format("added: %s", 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.
Could use some javadocs documentation as Task is the parent class.
src/main/java/Duke.java
Outdated
for (int i = 1; i < arrWords.length; i++) { | ||
if (arrWords[i].equals("/at")) { | ||
for (int j = i + 1; j < arrWords.length; j++) { | ||
dayAndTime = dayAndTime + " " + arrWords[j]; | ||
} | ||
break; | ||
} else { | ||
remainingWords = remainingWords + " " + arrWords[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.
Could use comments in places like these to explain what the code is doing (i.e. splitting up the user input) as it may not be apparent on first sight.
src/main/java/Action.java
Outdated
void showList(ArrayList<Task> arrlst) { | ||
System.out.println("Here are the tasks in your list:"); | ||
for (int i = 0; i < arrlst.size(); i++) { | ||
String output = String.format("%d.[%s][%s]%s\n", i + 1, arrlst.get(i).sym, | ||
arrlst.get(i).getStatusIcon(), arrlst.get(i).description); | ||
System.out.println(output); | ||
} | ||
} |
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 use a javadocs documentation.
src/main/java/Event.java
Outdated
@Override | ||
String getSym() { | ||
return this.sym; | ||
} |
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.
May I check what is the purpose of this override? Same with deadline and todo classes. Seems redundant as the getSym() in Task is the same.
src/main/java/Task.java
Outdated
@@ -0,0 +1,45 @@ | |||
public class 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.
While protected may appear sufficient, these attributes can still be accessible by other functions once you start putting them into packages. Consider changing your attributes to private
src/main/java/Task.java
Outdated
|
||
@Override | ||
public String toString() { | ||
return String.format("added: %s", 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.
I think you should change your toString() method to print out the sym
, isDone
and description
instead of this message. This can reduce code duplication in your children classes as I observed that they are using attributes in Task
to form the string.
src/main/java/Task.java
Outdated
System.out.println(output); | ||
} | ||
|
||
String getSym() { |
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.
Missing public
keyword at function signature
Good effort overall. Some improvements to consider:
|
# Conflicts: # src/main/java/duke/Parser.java
Assert that isDone must be false to get " " from getStatusIcon() method.
Add update into help function
Duke frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
its FREE
tasklist:
Here's the main method: