-
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
[yuhengr] ip #193
base: master
Are you sure you want to change the base?
[yuhengr] ip #193
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.
Nice work! Overall, the code follows the code quality guidelines. Naming of variables & methods are appropriate, and the code is readable with the exception of some long lines.
src/main/java/Duke.java
Outdated
int fromPosition = userInput.indexOf("/from"); | ||
int toPosition = userInput.indexOf("/to"); | ||
|
||
tasks[taskCount] = new Event(userInput.substring(dividerPosition + 1, fromPosition - 1), userInput.substring(fromPosition + 6, toPosition - 1), userInput.substring(toPosition + 4)); |
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 can consider line wrapping? This line seems quite long.
src/main/java/Duke.java
Outdated
else if(words[0].equals("deadline")){ | ||
int dividerPosition = userInput.indexOf(" "); | ||
int slashPosition = userInput.indexOf("/by"); | ||
tasks[taskCount] = new Deadline(userInput.substring(dividerPosition + 1, slashPosition - 1), userInput.substring(slashPosition + 4)); |
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 can consider line wrapping for this line also?
src/main/java/Duke.java
Outdated
|
||
String userInput; | ||
Scanner in = new Scanner(System.in); | ||
Task[] tasks = new Task[100]; |
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 like the use of plural form to indicate multiple tasks
src/main/java/Duke.java
Outdated
@@ -6,5 +8,72 @@ public static void main(String[] args) { | |||
+ "| |_| | |_| | < __/\n" | |||
+ "|____/ \\__,_|_|\\_\\___|\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 the logo variable be deleted since it's not used?
src/main/java/Task.java
Outdated
public String getStatusIcon() { | ||
return (isDone ? "X" : " "); | ||
} | ||
|
||
public void markAsDone() { | ||
this.isDone = true; | ||
} | ||
|
||
public void markAsNotDone() { | ||
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.
Method names are good as they sound like verbs
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.
Hi Yu Heng,
Try to avoid using magic literals in your program, this is a code quality requirement!
Do consider refactoring some parts of your code that are very long. This will help in readability!
There are some other specific comments below. Try and fix them before the next tutorial!
For your consideration: There is a way to autoformat your code automatically every time you Ctrl-S on intellij which will save you much pain. :)
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,14 @@ | |||
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.
Formatting error with the curly brace!
src/main/java/Duke.java
Outdated
public class Duke { | ||
public static void main(String[] args) { | ||
/* | ||
String logo = " ____ _ \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.
Ditto with the comment by your peer. Remove code that is commented out!
src/main/java/Duke.java
Outdated
String userInput; | ||
Scanner in = new Scanner(System.in); | ||
ArrayList<Task> tasks = new ArrayList<>(); | ||
// Task[] tasks = new Task[100]; |
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.
Remove unused code, please.
src/main/java/Duke.java
Outdated
while(!userInput.equals("bye")){ | ||
String[] userInputWords = userInput.split(" "); | ||
|
||
if(userInput.equals("list")){ |
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.
This if-else chain is nested and very long. This usually hints that there can be room for improvement for making your code SLAP more.
To aid you in achieving SLAP, here are some guiding questions...
- What parts of the code is repetitive? i.e. virtually the same for each if/else block you have
- What part of the code does a particular function that can be isolated into a function?
src/main/java/Duke.java
Outdated
int fromPosition = userInput.indexOf("/from"); | ||
int toPosition = userInput.indexOf("/to"); | ||
|
||
tasks.add(new Event(userInput.substring(dividerPosition + 1, fromPosition - 1), userInput.substring(fromPosition + 6, toPosition - 1), userInput.substring(toPosition + 4))); |
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.
This is pretty hard to read, especially with all the magic literals. Do refactor your magic literals. You can consider breaking this up into a function!
src/main/java/Event.java
Outdated
@@ -0,0 +1,15 @@ | |||
public class Event 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.
Formatting error with the curly brace!
Add Level-9 in branch-Level-9
Add A-JavaDoc
No description provided.