-
Notifications
You must be signed in to change notification settings - Fork 462
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
[Lik Hern] iP #488
base: master
Are you sure you want to change the base?
[Lik Hern] iP #488
Conversation
src/main/java/Duke.java
Outdated
break; | ||
case BYE: | ||
System.out.println("Bye. Hope to see you again soon!"); | ||
break UserInput; |
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.
Interesting use of Java labels!
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 a good job. Code is simple and easy to understand and follow. Naming of variables are also appropriate.
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 job! However, you can write more javadocs.
src/main/java/Event.java
Outdated
this.at = at; | ||
} | ||
|
||
@Override |
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 write java docs
src/main/java/Task.java
Outdated
return isDone ? "X" : " "; | ||
} | ||
|
||
public void markAsDone() { |
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 write java docs
src/main/java/Deadline.java
Outdated
|
||
@Override | ||
public String toString() { | ||
return String.format("[D]%s (by: %s)", super.toString(), by.format(DateTimeFormatter.ofPattern("MMM dd yyyy"))); |
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.
Line too long, can break line in the middle
src/main/java/Deadline.java
Outdated
import java.time.format.DateTimeFormatter; | ||
|
||
public class Deadline extends Task { | ||
protected LocalDate 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.
Maybe you could name the variable as "deadLineTime" instead of "by"?
src/main/java/Event.java
Outdated
@@ -0,0 +1,18 @@ | |||
public class Event extends Task { | |||
protected String at; |
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 like the Deadline class, perhaps you can change the variable name from "at" to "eventTime"?
Follows good OO practices, and good naming of variables. Related classes have not been placed in the same folder (package) and more JavaDocs could have been added. |
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.
Nice work so far, good luck with your iP!
src/main/java/Duke.java
Outdated
case DEADLINE: | ||
scanner.useDelimiter("/by"); | ||
String deadlineDescription = scanner.next().strip(); | ||
scanner.reset().skip("/by"); | ||
String deadlineBy = scanner.nextLine().strip(); | ||
Deadline deadlineTask = new Deadline(deadlineDescription, deadlineBy); | ||
storage.add(deadlineTask); | ||
System.out.println("Got it. I've added this task:\n" + deadlineTask | ||
+ "\nNow you have " + storage.size() + " tasks in the list."); | ||
break; | ||
case EVENT: | ||
scanner.useDelimiter("/at"); | ||
String eventDescription = scanner.next().strip(); | ||
scanner.reset().skip("/at"); | ||
String eventAt = scanner.nextLine().strip(); | ||
Event eventTask = new Event(eventDescription, eventAt); | ||
storage.add(eventTask); | ||
System.out.println("Got it. I've added this task:\n" + eventTask | ||
+ "\nNow you have " + storage.size() + " tasks in the list."); | ||
break; |
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.
Code seems to repeat the concept of finding some delimiter, perhaps this can be abstracted?
src/main/java/Task.java
Outdated
|
||
protected Task(String description) { | ||
this.description = description; | ||
this.isDone = false; |
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 suggests avoiding unnecessary use of this
with fields. (isDone
here, in particular.)
https://se-education.org/guides/conventions/java/index.html#variables
# Conflicts: # src/main/java/duke/ui/Ui.java
# Conflicts: # src/main/java/duke/task/Task.java
Include assertions to verify certain assumptions
Do refactor to improve code quality
DukePro
DukePro frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features:
If you are Java programmer, you can use it to practice Java too. Here's the
main
method: