-
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
[Dominic Siew Zhen Yu] iP #494
base: master
Are you sure you want to change the base?
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 it's fine! just a bit of naming conventions but otherwise looks good 👍
src/main/java/duke/Deadlines.java
Outdated
import java.time.LocalDate; | ||
import java.time.format.DateTimeFormatter; | ||
|
||
public class Deadlines 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.
You might want to change the class name to Deadline
instead since plural forms are usually for a collection of data such as an array of deadlines. 😄
src/main/java/duke/DukeMan.java
Outdated
String command = parser.getCommand(); | ||
|
||
switch(command) { | ||
case "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.
the indentation for case
should be the same as switch
* | ||
* @return a string representation of a Event object | ||
*/ | ||
public String printName() { |
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.
instead of printName
, you might want to override the toString
method instead since your method technically does not print the name
src/main/java/duke/taskList.java
Outdated
import java.io.*; | ||
import java.util.ArrayList; | ||
|
||
public class taskList { |
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.
class name should've been capitalised instead -- so TaskList
instead of taskList
?
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 Dominic! Your code looks good overall but there are just a few naming and coding style issues that you should fix. Cheers!
src/main/java/duke/DukeMan.java
Outdated
String todoName = parser.getTaskName(); | ||
tasks.addTodo(todoName, true); | ||
break; | ||
case "deadline": |
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.
case
statement should have the same indentation level as switch
* @param input the given input by the user | ||
*/ | ||
|
||
public void parsing(String input) { |
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 a better name for this method will be parse
? I think we generally should avoid adding '-ing' behind our methods and class names.
* @author Dominic Siew Zhen Yu | ||
*/ | ||
|
||
public class DukeMan { |
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 can name your class name in a way that explains what this class is supposed to do, as per the naming guidelines
src/main/java/duke/Deadlines.java
Outdated
import java.time.LocalDate; | ||
import java.time.format.DateTimeFormatter; | ||
|
||
public class Deadlines 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.
You may want to change your class name to singular form (deadline
) as plural forms are usually for collection of items.
src/main/java/duke/taskList.java
Outdated
import java.io.*; | ||
import java.util.ArrayList; | ||
|
||
public class taskList { |
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.
As part of convention, your class name should have been capitalised. It should be TaskList
instead of taskList
src/main/java/duke/taskList.java
Outdated
boolean isCompleted = (seperate2[0].equals("[✓]"))? true: false; | ||
String eventInfo = seperate2[1]; | ||
|
||
switch(taskTypeText) { |
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.
Same indentation problem with switch
and case
src/main/java/duke/taskList.java
Outdated
|
||
while ((currentLine = reader.readLine()) != null) { | ||
String[] seperate1 = currentLine.toString().split(" ", 2); | ||
String taskTypeText = seperate1[0]; |
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 a better name would be taskString
?
src/main/java/duke/Duke.java
Outdated
// | ||
///** | ||
// * Duke is a chatbot that takes in tasks, which includes todos, deadlines and events, and compiles them | ||
// * in a list. Tasks can be removed from the list and be marked as completed. |
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.
Just a friendly reminder that there should not be large chunks of commented code present as mentioned in the iP grading rubrics. You should delete them if not needed or transfer them somewhere else 😄
* commit 'd6bdc6f3d02334bc5b96ca48ec75a10c78365ca3': Update README.md Update README.md Set theme jekyll-theme-cayman Update README.md Update README.md Update README.md Update README.md Update README.md Update README.md Update README.md # Conflicts: # docs/README.md
DukeMan
Manage your tasks with DukeMan, an all-in-one task management application. 🙂
FASTSUPER FAST and easy to useAll you need to do to use DukeMan 👀
DOWNLOAD NOW!
If you Java programmer, you can use it to practice Java too. Here's the
main
method: