-
Notifications
You must be signed in to change notification settings - Fork 187
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
[LinSiyan]iP #181
base: master
Are you sure you want to change the base?
[LinSiyan]iP #181
Conversation
Dear sir/madam, Sorry to bother, however I tried to PR my master branch after I finished the level 1 2 and 3, but I can't find my pushed changes on my remote master branch, the updates only show up at the ipMyAnswer branch, however when I pushed in SourceTree, I chose both branches to push. So is there any way for me to show my updates on my master branches and send PR using my master branch? I will be grateful if you can do me this favor! Yours, |
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 need to avoid using magic numbers and apply abstraction to improve the readability of the main function.
src/main/java/dukeRobot/Duke.java
Outdated
@@ -0,0 +1,104 @@ | |||
import 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.
Avoid importing *, should explicitly import the classes
src/main/java/dukeRobot/Duke.java
Outdated
Task[] tasks = new Task[100]; | ||
while (!userInput.contains("bye")) { | ||
|
||
System.out.println("____________________________________________________________\n"); |
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 make this a constant since this line will be reused multiple times. Any changes made will also easily reflected.
src/main/java/dukeRobot/Duke.java
Outdated
} else if (userInput.contains("mark")) { | ||
if (userInput.contains("unmark")) { | ||
|
||
indexOfMark = Integer.parseInt(userInput.split(" ")[1]) - 1; |
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.
Avoid use of magic number. Try to have a descriptive variable for them
src/main/java/dukeRobot/Duke.java
Outdated
import java.util.ArrayList; | ||
import java.util.List; | ||
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.
Very long function, more than 30 lines of code. Can consider abstracting out each 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.
Could you please be more specific about the abstracting? Thanks!
|
||
public void markAsDone() { | ||
this.isDone = true; | ||
System.out.println("OK, I've marked this task as not done yet:\n"); |
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.
System.out.println("OK, I've marked this task as not done yet:\n"); | |
System.out.println("OK, I've marked this task as done\n"); |
public class ToDo extends Task { | ||
public ToDo(String description) throws ArrayIndexOutOfBoundsException{ | ||
super(description); | ||
//System.out.println(description + "yes"); |
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.
Try to remove unused comments
level9. Add find and Merge pull request
[Level 0: Rename, Greet, Exit]