-
Notifications
You must be signed in to change notification settings - Fork 461
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
[Tin Jingyao] iP #507
base: master
Are you sure you want to change the base?
[Tin Jingyao] iP #507
Conversation
There are 3 types of tasks, Todos, Events, and Deadline 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.
Generally good effort!
I like the clear naming of classes, variables and methods.
I think generally you could split your code into different classes, and include JavaDocs for your classes and methods!
src/main/java/Duke.java
Outdated
import java.util.ArrayList; | ||
import java.util.Locale; | ||
import java.util.Scanner; | ||
|
||
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.
Perhaps apply OOP to split the Duke class into different smaller classes?
src/main/java/Duke.java
Outdated
|
||
private static final ArrayList<Task> LIST_OF_THINGS = new ArrayList<>(); | ||
|
||
private abstract static 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.
Maybe you could create Task as a separate class, instead of nesting it in the Duke class.
src/main/java/Duke.java
Outdated
} | ||
} | ||
|
||
private static class EventTask extends 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 the different Tasks clearly! 👍
src/main/java/Duke.java
Outdated
return false; | ||
} | ||
try { | ||
Integer.parseInt(str.substring(7)); |
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 could consider using str.split(" ") rather than str.substring(7). May improve reusability of code instead of having to change the number according to the length of the command?
src/main/java/Duke.java
Outdated
public class Duke { | ||
|
||
private static final ArrayList<Task> LIST_OF_THINGS = new ArrayList<>(); |
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 you could consider extracting this out as a separate TaskList class, which would handle adding tasks, etc.
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.
Coding standard is generally adhered to w.r.t naming, but could extract out the different classes, as well as add JavaDocs. :)
src/main/java/Duke.java
Outdated
import java.util.ArrayList; | ||
import java.util.Locale; | ||
import java.util.Scanner; | ||
|
||
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.
Could split the class into various classes, i.e. TaskList, Event, Deadline, instead of putting them into one class
src/main/java/Duke.java
Outdated
private static final ArrayList<Task> LIST_OF_THINGS = new ArrayList<>(); | ||
|
||
private abstract static class Task { | ||
private final String task_name; |
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 be camelcase formatting for variables
src/main/java/Duke.java
Outdated
import java.util.ArrayList; | ||
import java.util.Locale; | ||
import java.util.Scanner; | ||
|
||
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.
Could also add JavaDocs for documentation purposes
Shifted inner class Task to a new file
src/main/java/duke/Command.java
Outdated
public class Command { | ||
|
||
} |
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 am curious why is this empty?
# Conflicts: # src/main/java/duke/Duke.java # src/main/java/duke/task/TodoTask.java
# Conflicts: # src/main/java/duke/Parser.java
# Conflicts: # text-ui-test/EXPECTED.TXT # text-ui-test/input.txt
Let's shorten excessively long methods by abstracting out certain parts into separate methods.
Add assert statements
# Conflicts: # src/main/java/duke/Parser.java
Branch a-code-quality
Flexibility to change the local file path through Duke allows users to set new file locations without restarting Duke. Users may also create new files this way.
Add some JavaDocs
DukePro
DukePro frees your mind of having to remember things you need to do. It's,
text-based
easy to learn
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features:
If you Java programmer, you can use it to practice Java too. Here's the
main
method: