-
Notifications
You must be signed in to change notification settings - Fork 454
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
[Lulu Y] iP #473
base: master
Are you sure you want to change the base?
[Lulu Y] iP #473
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, your code quality is pretty good in my opinion. Just some things to take note, I think you should start adding spaces within your statements as it is part of the Java coding standard. Maybe also consider declaring class variables as private, unless you have a reason to do otherwise.
src/main/java/Duke.java
Outdated
f.createNewFile(); | ||
} | ||
Scanner sc = new Scanner(f); | ||
ArrayList<Task> t = 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.
I am not sure I like the name of the ArrayList. I think giving it a clearer name would improve the readability of your code. For example, 'taskList'.
src/main/java/Duke.java
Outdated
int ctr = 0; | ||
Scanner scanner = new Scanner(System.in); | ||
System.out.println("_______________________________________________"); | ||
System.out.println("Hello! I'm Duke\n" + "What can I do for you?"); |
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 placing a line break before the '+' would improve improve the readability of your code. This is because you add a next line character before the '+', so presenting your code in the same way the output will look would be nice.
src/main/java/Duke.java
Outdated
System.out.println(d.getMessage()); | ||
} | ||
} | ||
}catch(IOException e){ |
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 you may have missed this. Add a space between the } and catch.
src/main/java/Duke.java
Outdated
text = text.concat("0|"); | ||
} | ||
text = text.concat(t.description + "|" + ((Event) t).by + "\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.
I think it is better to add a space around every 'if', 'else' and 'for' as this helps the readability of your code.
src/main/java/DukeException.java
Outdated
@@ -0,0 +1,5 @@ | |||
public class DukeException extends Exception{ |
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 you should add a space after Exception. I noticed that this is a recurring practice for your classes. Adding a space between the class declaration and the { will help to add readability to your code. I suggest you read the part on "White space within a statement" in the Java coding standard 😄 .
src/main/java/Event.java
Outdated
@@ -0,0 +1,15 @@ | |||
public class Event 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.
I think it would be better to keep your class variables as private. Do you have a reason to declare them as protected?
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.
Other than the comments from the other reviewers and myself, LGTM!
src/main/java/Deadline.java
Outdated
import java.time.LocalDateTime; | ||
import java.time.format.DateTimeFormatter; | ||
|
||
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.
Could perhaps add header docs to the class!
src/main/java/Duke.java
Outdated
} | ||
int ctr = 0; | ||
Scanner scanner = new Scanner(System.in); | ||
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.
Can perhaps use a function to print the dividers to make the code look more readable!
Certain Assumptions in the code needs to be verified. Let's utilize Java Assertions and use the keyword assert to verify assumptions in the code.
Branch A assertion
Long methods are hard to read and affect code quality. Let's follow the coding standards and break long methods by refractoring.
Some methods in the code are too long in the Parser and Storage class.
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 Java programmer, you can use it to practice Java too. Here's the
main
method: