-
Notifications
You must be signed in to change notification settings - Fork 270
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
[KristopherPTaslim] iP #287
base: master
Are you sure you want to change the base?
[KristopherPTaslim] iP #287
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 you are following the coding standards, very good job! 💯 just a few minor mistakes made, after you change it, it'll all look good to me 👏
src/main/java/Duke.java
Outdated
import java.util.ArrayList; | ||
import java.util.Scanner; | ||
import java.io.IOException; | ||
import java.time.LocalDateTime; | ||
import java.time.format.DateTimeFormatter; |
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.
Following the coding standards, you might need to arrange the import statements into alphabetical order! :)
Same for some of the other classes that includes import statements.
import java.util.ArrayList; | |
import java.util.Scanner; | |
import java.io.IOException; | |
import java.time.LocalDateTime; | |
import java.time.format.DateTimeFormatter; | |
import java.io.IOException; | |
import java.time.format.DateTimeFormatter; | |
import java.time.LocalDateTime; | |
import java.util.ArrayList; | |
import java.util.Scanner; |
src/main/java/Duke.java
Outdated
|
||
class Duke { | ||
|
||
public static void main(String[] args) throws DukeException{ |
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 need to leave a space after DukeException
public static void main(String[] args) throws DukeException{ | |
public static void main(String[] args) throws DukeException { |
src/main/java/Duke.java
Outdated
FileClass fc = new FileClass(); //file class | ||
String home = System.getProperty("user.home"); //home directory |
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 make comments behind the lines! It makes the logic clear 😄
src/main/java/Duke.java
Outdated
String dateTime = input.substring(indexOfTime + deadlineCondition.length(), input.length()); // the date and time for by | ||
//convert to the correct one | ||
LocalDateTime deadlineTime = LocalDateTime.parse(dateTime, DateTimeFormatter.ofPattern("d/M/y Hmm")); | ||
String convertedTime = deadlineTime.format(DateTimeFormatter.ofPattern("MMM d yyyy hh:mm a")); |
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 name your variables :), it is very clear what they all do 👍
src/main/java/Duke.java
Outdated
|
||
// Recording everything in the duke.txt | ||
for (int i = 0; i < taskArray.size(); i++ ) { | ||
Task tasks = taskArray.get(i); |
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 it is referring to just 1 task, will it be better to name this variable as 'task' instead of 'tasks'?
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.
Great code quality! The variable names used are all short and descriptive and help a lot to make the code easy to understand! 😄
src/main/java/Duke.java
Outdated
int indexOfTime = input.indexOf(eventCondition); //to find / | ||
String dateTime = input.substring(indexOfTime + eventCondition.length(), input.length()); // the date and time for by | ||
//convert to the correct one | ||
LocalDateTime eventTime = LocalDateTime.parse(dateTime, DateTimeFormatter.ofPattern("d/M/y Hmm")); | ||
String convertedTime = eventTime.format(DateTimeFormatter.ofPattern("MMM d yyyy hh:mm a")); | ||
String stringSliced = input.substring(6, indexOfTime); // after deadline | ||
stringArray.add(stringSliced); | ||
Event eventTask = new Event(stringSliced, convertedTime); | ||
taskArray.add(eventTask); | ||
String noOfTask = String.valueOf(taskArray.size()); | ||
System.out.println(messageTask + eventTask.toString() + "\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.
Great use of comments to clarify what each line is doing! Maybe you could also use a few blank lines to separate the code block into logical sections (such as formatting, processing and output.) The coding standard recommends that "Logical units within a block should be separated by one blank line."
src/main/java/Duke.java
Outdated
System.out.println(index + "." + taskArray.get(i)); | ||
} | ||
} | ||
} else if (checkType[0].equals("mark") || checkType[0].equals("unmark")) { //this is to check whether it goes through marking |
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 this line has more than the 120 character line limit allowed by the coding standard. Maybe you could add a line break to improve readability
src/main/java/Duke.java
Outdated
|
||
try { | ||
int index = Integer.parseInt(checkType[1]) - 1; | ||
Task tasks = taskArray.get(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.
Maybe change the name of this variable to task
since it only refers to a single entry
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 you adhered mostly to the coding standards, keep it up!
src/main/java/Parser.java
Outdated
default: | ||
ui.showDefaultMessage(); | ||
break; | ||
|
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 remove the indentation for the case clauses to follow the coding standards
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 Kris! Code is generally well done, just keep in mind that it's best to break up code into smaller blocks for increased readability and to use more descriptive naming:)
public Deadline(String description, String by) { | ||
super(description); | ||
this.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.
Perhaps you could rename "by" to something more descriptive.
public Deadline(String description, String by) { | |
super(description); | |
this.by = by; | |
} | |
public Deadline(String description, String deadlineDate) { | |
super(description); | |
this.deadlineDate = deadlineDate; | |
} |
src/main/java/duke/Duke.java
Outdated
Parser parser = new Parser(taskList); | ||
|
||
while (sc.hasNextLine()) { | ||
parser.execute(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 you can change the name of the method since a parser is parsing an input
parser.execute(input); | |
parser.parseInput(input); |
public DukeException(String Message) { | ||
super(Message); | ||
} |
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 could add a more descriptive message to be stored within the DukeException class
public Event(String description, String at) { | ||
super(description); | ||
this.at = at; | ||
} |
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 thing with the deadline task
public Event(String description, String at) { | |
super(description); | |
this.at = at; | |
} | |
public Event(String description, String eventDate) { | |
super(description); | |
this.eventDate= eventDate; | |
} |
src/main/java/duke/Parser.java
Outdated
Task tasks = taskList.getTaskArray().get(index); | ||
System.out.println(tasks.marking(checkCase[0].toLowerCase())); | ||
taskList.getTaskArray().set(index, tasks); | ||
} catch (ArrayIndexOutOfBoundsException e) { | ||
ui.showInvalidInput(); | ||
} | ||
break; | ||
|
||
case ("unmark"): | ||
try { | ||
int index = Integer.parseInt(checkCase[1]) - 1; | ||
Task tasks = taskList.getTaskArray().get(index); | ||
System.out.println(tasks.marking(checkCase[0].toLowerCase())); | ||
taskList.getTaskArray().set(index, tasks); | ||
} catch (ArrayIndexOutOfBoundsException e) { | ||
ui.showInvalidInput(); | ||
} | ||
break; | ||
|
||
case ("todo"): | ||
|
||
try { | ||
String toDoCondition = "todo "; | ||
int indexOfToDo = toDoCondition.length(); //to find todo | ||
String stringSliced = input.substring(indexOfToDo,input.length()); | ||
Todo todoTask = new Todo(stringSliced); | ||
taskList.addTask(todoTask); | ||
String noOfTask = String.valueOf(taskList.getTaskArray().size()); | ||
ui.showAddedMessage(todoTask, noOfTask); | ||
} catch (StringIndexOutOfBoundsException e) { | ||
ui.showTodoError(); | ||
} | ||
break; | ||
|
||
case ("deadline"): | ||
|
||
try { | ||
String deadlineCondition = "/by "; | ||
int indexOfTime = input.indexOf(deadlineCondition); //to find / | ||
String dateTime = input.substring(indexOfTime + deadlineCondition.length(), input.length()); // the date and time for by | ||
//convert to the correct one | ||
LocalDateTime deadlineTime = LocalDateTime.parse(dateTime, DateTimeFormatter.ofPattern("d/M/y Hmm")); | ||
String convertedTime = deadlineTime.format(DateTimeFormatter.ofPattern("MMM d yyyy hh:mm a")); | ||
String stringSliced = input.substring(9, indexOfTime); // after deadline | ||
Deadline deadlineTask = new Deadline(stringSliced, convertedTime); | ||
taskList.addTask(deadlineTask); | ||
String noOfTask = String.valueOf(taskList.getTaskArray().size()); | ||
ui.showAddedMessage(deadlineTask, noOfTask); | ||
} catch (StringIndexOutOfBoundsException e) { | ||
ui.showDeadlineError(); | ||
} | ||
break; | ||
|
||
case ("event"): | ||
|
||
try { | ||
String eventCondition = "/at "; | ||
int indexOfTime = input.indexOf(eventCondition); //to find / | ||
String dateTime = input.substring(indexOfTime + eventCondition.length(), input.length()); // the date and time for at | ||
//convert to the correct one | ||
LocalDateTime eventTime = LocalDateTime.parse(dateTime, DateTimeFormatter.ofPattern("d/M/y Hmm")); | ||
String convertedTime = eventTime.format(DateTimeFormatter.ofPattern("MMM d yyyy hh:mm a")); | ||
String stringSliced = input.substring(6, indexOfTime); // after deadline | ||
Event eventTask = new Event(stringSliced, convertedTime); | ||
taskList.addTask(eventTask); | ||
String noOfTask = String.valueOf(taskList.getTaskArray().size()); | ||
ui.showAddedMessage(eventTask, noOfTask); | ||
|
||
} catch (StringIndexOutOfBoundsException e) { | ||
ui.showEventError(); | ||
} | ||
break; | ||
|
||
case("delete"): | ||
|
||
try { | ||
int index = Integer.parseInt(checkCase[1]) - 1; | ||
Task task = taskList.getTaskArray().get(index); | ||
taskList.deleteTask(index); | ||
String noOfTask = String.valueOf(taskList.getTaskArray().size()); | ||
ui.showDeletedMessage(task, noOfTask); | ||
} catch (ArrayIndexOutOfBoundsException e) { | ||
ui.showDeleteError(); | ||
} | ||
break; | ||
|
||
case("bye"): | ||
ui.showGoodbyeMessage(); | ||
break; | ||
|
||
default: | ||
ui.showDefaultMessage(); | ||
break; | ||
|
||
|
||
|
||
} | ||
|
||
|
||
} |
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.
Apart from the coding standard violations, perhaps you can break up each case instruction into separate methods for increased readability?
For example:
public void delete() {
try {
int index = Integer.parseInt(checkCase[1]) - 1;
Task task = taskList.getTaskArray().get(index);
taskList.deleteTask(index);
String noOfTask = String.valueOf(taskList.getTaskArray().size());
ui.showDeletedMessage(task, noOfTask);
} catch (ArrayIndexOutOfBoundsException e) {
ui.showDeleteError();
}
}
The method execute() does not follow the coding standard due to its length. There are common logic in the method which cause code duplication. Extracting this common logic allows the method to be significantly shorter and the principle of SLAP to be followed. For example, the method parseByCondition() and parseString() shows the SLAP principle.
Assertions are required in order to check whether the code is able to work. Assertions allow assertion error to be thrown if some conditions are not held properly. For example, implementing assertion task cannot be null in the method showAddedMessage(Task task, String noOfTask) will throw assertion error if the input of task is null.
Add assertions in some various points in code
Edit execute() method in Parser class to follow coding standard.
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: