-
Notifications
You must be signed in to change notification settings - Fork 435
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
[quelinxiao] ip #477
base: master
Are you sure you want to change the base?
[quelinxiao] ip #477
Conversation
Let's tweak the docs/README.md (which is used as the user guide) to fit Duke better. Specifically, 1. mention product name in the title 2. mention adding a product screenshot and a product intro 3. tweak the flow to describe feature-by-feature
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.
LGTM. Small nits
src/main/java/Bob.java
Outdated
@@ -31,7 +31,7 @@ else if (input.equals("list")) { | |||
} | |||
} | |||
|
|||
else if (input.trim().matches("mark|unmark|deadline|todo|event")) { | |||
else if (input.trim().matches("mark|unmark|deadline|todo|event|delete")) { |
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.
Small nit. I believe the else if should be preceded by the closing curly brackets from the if/if else statement above 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.
Looks great to merge. Just small nits
src/main/java/Bob.java
Outdated
String greet = " Hello! I'm Bob.\n" | ||
+ " What can I do for you?\n"; | ||
|
||
String exit = " Bye. Hope to see you again soon!"; |
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 changing this to a final String as the value of the greeting and exit phrase will not change
src/main/java/Bob.java
Outdated
+ " Now you have " + taskList.size() + " tasks in the list."); | ||
} | ||
|
||
else if (input.startsWith("todo ")) { |
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.
Having spaces at the end of the string does not look very clean. Maybe can consider removing that.
src/main/java/Bob.java
Outdated
*/ | ||
@Override | ||
public String toString() { | ||
return "[D]" + super.toString() + " (by: " + 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.
Clean way to handle output. I did not know that!
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.
Good attempt however please try and separate out your classes, they should not all be in the same java file. There's also almost no use of OOP here, you can create utility classes for handling Ui, Exceptions, Files, instead of storing them all inside methods.
data/bob.txt
Outdated
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.
Local data files like this should not be pushed onto remote
src/main/java/Bob.java
Outdated
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 consider separating your code into different classes and packages :)
src/main/java/Bob.java
Outdated
/* | ||
* This class represents a task we want to record. | ||
*/ | ||
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.
Different Classes should not be in the same .java file. Consider separating them into dedicated java files
src/main/java/Bob.java
Outdated
* | ||
* @return A boolean depending on whether the task is done. | ||
*/ | ||
public boolean getIsDone() { |
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.
By java convention, methods that start with get() usually return a Value like a string, integer or array. For booleans you can just use isDone()
src/main/java/Bob.java
Outdated
|
||
/* | ||
* A constructor that depicts a new 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.
Since Task is never initiated and instead inherited by other subclasses, consider using an abstract class instead.
src/main/java/Bob.java
Outdated
} | ||
|
||
catch (IOException e) { | ||
System.out.println(e.getMessage()); |
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 basic way of handling exceptions. Consider creating a new Exception class so you can consolidate your exceptions and have different behavior for different types of Exceptions. This method of doing things just means that when your program really does have an exception, it'll be hard to pinpoint where or how it came from because the try block is so big
# Conflicts: # src/main/java/bob/Bob.java
Main class was set to Bob so the GUI would not work Changing main class to Launcher allows it to be the entry point of the application
Include assertions in code
Improve code quality
Bob
Why
you
should use Bob:FASTSUPER FAST to useSteps to take:
It is completely FREE!
Features:
This is the main method: