-
Notifications
You must be signed in to change notification settings - Fork 116
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
[Atul Teja Vellampalli] iP #9
base: master
Are you sure you want to change the base?
Conversation
…e and ran the tests
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 job on OOP! The code is neat with good variable names and personalisation However, do take note of the length of characters in a single line.
src/main/java/JeM.java
Outdated
System.out.println(" Hello! I'm JeM, Your e-Assistant"); | ||
System.out.println(" Personal To-Do list bot! "); | ||
System.out.println(" Just type out your tasks you have to complete and I will make a list of them for you."); | ||
System.out.println(" Type 'list' to see the current list of tasks, and type 'delete <task number>' to delete that task."); | ||
System.out.println(" Finally, type 'bye' to end the chat!"); |
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 how you provided instructions for using this chatbot
src/main/java/Storage.java
Outdated
int index = 0; | ||
for (Task task : taskList) { | ||
if (task instanceof Deadline) { | ||
System.out.println((index + 1) + "." + task.getStatusIcon() + " " + task + " (by: " + ((Deadline) task).getDeadline() + ")"); |
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.
Perhaps you could do some line wrapping?
src/main/java/Storage.java
Outdated
for (Task task : taskList) { | ||
if (task instanceof Deadline) { | ||
System.out.println((index + 1) + "." + task.getStatusIcon() + " " + task + " (by: " + ((Deadline) task).getDeadline() + ")"); | ||
}else if (task instanceof Event) { | ||
System.out.println((index + 1) + "." + task.getStatusIcon() + " " + task + " (from: " + ((Event) task).getStart() + " to: " + ((Event) task).getEnd() + ")"); | ||
}else { | ||
System.out.println((index + 1) + "." + task.getStatusIcon() + " " + task); | ||
} | ||
index++; | ||
} |
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.
Extra indentation found here
src/main/java/JeM.java
Outdated
while (true) { | ||
String line = scanner.nextLine().trim(); | ||
|
||
if (line.equalsIgnoreCase("bye")) { | ||
System.out.println("Bye. Hope to see you again soon!"); | ||
break; | ||
} | ||
|
||
handleInput(line, storage); | ||
} |
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.
Would it be better to extract this function for better abstraction?
src/main/java/JeM.java
Outdated
handleInput(line, storage); | ||
} | ||
|
||
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.
It will be better if you could extract this line of code to create a printLine() method, so you wouldn't have to copy and paste this line throughout the code.
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,17 @@ | |||
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.
Good coding practices for this file and proper naming conventions followed.
However, it could use more comments for this file, but the code is clean and easy to understand.
src/main/java/Event.java
Outdated
@@ -0,0 +1,25 @@ | |||
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.
Clean Code and proper naming conventions. However, there is unnecessary code that is commented out that can be deleted. Some comments can be added but the code is very clean and can be understood very easily.
src/main/java/JeM.java
Outdated
|
||
System.out.println("____________________________________________________________"); | ||
|
||
System.out.println(" Hello! I'm JeM, Your e-Assistant"); |
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.
Was a cute touch that you added instructions for the chatbot
src/main/java/JeM.java
Outdated
handleInput(line, storage); | ||
} | ||
|
||
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.
Would be better to either create a lineMessage() method or a String constant to neaten up the code
src/main/java/JeM.java
Outdated
Task task = new Event(taskContent, start, end); | ||
storage.storageInsert(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.
Code while being self explanatory could still use some comments on what some methods do
src/main/java/Storage.java
Outdated
int index = 0; | ||
for (Task task : taskList) { | ||
if (task instanceof Deadline) { | ||
System.out.println((index + 1) + "." + task.getStatusIcon() + " " + task + " (by: " + ((Deadline) task).getDeadline() + ")"); | ||
}else if (task instanceof Event) { | ||
System.out.println((index + 1) + "." + task.getStatusIcon() + " " + task + " (from: " + ((Event) task).getStart() + " to: " + ((Event) task).getEnd() + ")"); | ||
}else { | ||
System.out.println((index + 1) + "." + task.getStatusIcon() + " " + task); | ||
} | ||
index++; | ||
} | ||
} | ||
|
||
} |
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.
indentation is a bit off, please do check again
|
||
import java.util.Scanner; | ||
|
||
public class JeM { |
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.
You can consider using Jem instead to follow the PascalCase class naming format.
src/main/java/app/JeM.java
Outdated
} else if (line.toLowerCase().startsWith("delete")) { | ||
handleDeleteCommand(line, storage); | ||
} else if (line.toLowerCase().startsWith("unmark")) { | ||
handleUnmarkCommand(line, storage); | ||
} else if (line.toLowerCase().startsWith("mark")) { | ||
handleMarkCommand(line, storage); | ||
} else if (line.toLowerCase().startsWith("todo")) { | ||
handleTodoCommand(line, storage); | ||
} else if (line.toLowerCase().startsWith("deadline")) { | ||
handleDeadlineCommand(line, storage); | ||
} else if (line.toLowerCase().startsWith("event")) { | ||
handleEventCommand(line, storage); |
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.
.toLowerCase() seems to be used repetitively here. A switch case may be used instead.
if (task instanceof Deadline) { | ||
System.out.print(" (by: " + ((Deadline) task).getDeadline() + ")"); | ||
} else if (task instanceof Event) { | ||
System.out.print(" (from: " + ((Event) task).getStart() + " to: " + ((Event) task).getEnd() + ")"); |
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.
You can consider splitting this line to not reach the max character limit of 120.
Merged branch-Level-8 with master
branch-Level-9 merge
added JavaDoc documentation
No description provided.