-
Notifications
You must be signed in to change notification settings - Fork 228
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
[Jacob-109] iP #234
base: master
Are you sure you want to change the base?
[Jacob-109] iP #234
Conversation
e348e78
to
7e03915
Compare
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.
Keep it up Jacob! You can do it:)
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,14 @@ | |||
public class Deadline 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.
Should the class be added or grouped inside a package? This applies to other classes as well.
src/main/java/Duke.java
Outdated
Scanner sc = new Scanner(System.in); | ||
ArrayList<Task> list = new ArrayList<>(); | ||
|
||
while(sc.hasNext()) { |
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 to abstract and break this method into many smaller units? It will help in the future also.
src/main/java/Duke.java
Outdated
import java.lang.reflect.Array; | ||
import java.util.Scanner; | ||
import java.util.ArrayList; | ||
|
||
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.
Will it be better to add javadoc to all public classes and methods? The same applies to all other public classes and methods.
src/main/java/Duke.java
Outdated
@@ -1,10 +1,75 @@ | |||
import java.lang.reflect.Array; |
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.
Can I check if this import is being used? Because Array should be present by default in 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.
Name convention could have been better overall and some nitpicking.
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,14 @@ | |||
public class Deadline extends Task { | |||
|
|||
protected String 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.
You could have a better convention such as dateTime. "by" alone doesn't have any meaning.
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.
so sorry I thought this was my iP, please ignore my replies if any:)
src/main/java/Duke.java
Outdated
String logo = " ____ _ \n" | ||
+ "| _ \\ _ _| | _____ \n" | ||
+ "| | | | | | | |/ / _ \\\n" | ||
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
System.out.println("Hello from\n" + logo); | ||
Scanner sc = new Scanner(System.in); | ||
ArrayList<Task> list = 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.
Should be lists instead of list. Also, the naming convention could have been better, perhaps using tasks is better? After all, your code is being read by many people with different backgrounds. So it would be better if your naming convention is clearer.
src/main/java/Event.java
Outdated
@@ -0,0 +1,14 @@ | |||
public class Event extends Task { | |||
|
|||
protected String datetime; |
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 deadline by naming convention should have followed this format as well.
src/main/java/Duke.java
Outdated
|
||
while(sc.hasNext()) { | ||
String word = sc.nextLine(); | ||
if (word.equals("bye")) { |
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 change the if-else to switch case for better readability.
src/main/java/Duke.java
Outdated
} else if (word.equals("list")) { | ||
int size = list.size(); | ||
System.out.println("Here are the tasks in your list:"); | ||
for (int i = 1; i <= size; 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 have just do for (int i = 1; i <= list.size(); i++)
src/main/java/Duke.java
Outdated
public class Duke { | ||
public static void main(String[] args) { | ||
public static void main(String[] args) throws DukeException{ |
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 should catch the DukeException, if not your program would stop running due to the exception.
src/main/java/Task.java
Outdated
this.isDone = false; | ||
} | ||
|
||
public String 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.
It seems like getStatusIcon() is used in this task class only, hence you can change public to private.
The intended commits are those done on 21Jan 2021, i wanted to edit Level 3 but in the end it duplicated the commits above as well. I should have fixed the issue already. Sorry for the inconvenience caused if you saw the extra commits.