-
Notifications
You must be signed in to change notification settings - Fork 73
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
[Shannon Wong] iP #73
base: master
Are you sure you want to change the base?
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.
Overall, we found your code easy to read for the most parts except a few places
where the same constant was repeated. Hope our comments help you improve the code further.
Best of luck for your iP!!
src/main/java/Duke.java
Outdated
private static void printExitMessage() { | ||
System.out.println("____________________________________________________________"); | ||
System.out.println(" Bye. Hope to see you again soon!"); | ||
System.out.println("____________________________________________________________"); |
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.
Since you print the same line several times, how about making it into a constant and calling it?
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 do agree with this suggestion.
src/main/java/Duke.java
Outdated
* Prints out the welcome message | ||
*/ | ||
private static void printWelcomeMessage() { | ||
String logo = "Shannon"; |
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.
Since "Shannon" is a constant name and constant names must be all uppercase using underscore to separate words according to the coding standards, you can consider changing the name of the string from "logo".
src/main/java/Duke.java
Outdated
@@ -1,10 +1,116 @@ | |||
import java.util.Scanner; | |||
|
|||
public class Duke { | |||
public static void main(String[] args) { |
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 find your main method to be sufficient to be really good to fulfil all the requirements. May I know why you did it this way instead of creating a new Duke object inside the main class and calling all the methods for that object like I did.
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.
Over all, really nice code!
src/main/java/Task.java
Outdated
* @return either tick or cross | ||
*/ | ||
public String getStatusIcon() { | ||
return (isDone ? "\u2713" : "\u2718"); |
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 using constant rather than unicode. It may improve the readability of you code.
src/main/java/Duke.java
Outdated
System.out.println("____________________________________________________________"); | ||
System.out.println(" added: " + inputCommand); | ||
System.out.println("____________________________________________________________"); |
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 these three lines can be extracted to a method called something like "showFeedback"
src/main/java/Duke.java
Outdated
private static void printExitMessage() { | ||
System.out.println("____________________________________________________________"); | ||
System.out.println(" Bye. Hope to see you again soon!"); | ||
System.out.println("____________________________________________________________"); |
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 do agree with this suggestion.
# Conflicts: # src/main/java/duke/Duke.java
Add increment: A-JavaDoc
# Conflicts: # src/main/java/duke/Parser.java
Branch level 9
No description provided.