Skip to content

[Yeo Zong Yao] IP#171

Open
yeozongyao wants to merge 48 commits into
nus-cs2113-AY2324S2:masterfrom
yeozongyao:master
Open

[Yeo Zong Yao] IP#171
yeozongyao wants to merge 48 commits into
nus-cs2113-AY2324S2:masterfrom
yeozongyao:master

Conversation

@yeozongyao

Copy link
Copy Markdown

Contains all the changes and pushes done for all the IP levels to date

@yeozongyao yeozongyao changed the title Commits for Level-0 to A-CodingStandard Commits for Level-0 to Week 4 Feb 9, 2024

@hongyijie06 hongyijie06 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep up the good work!

Comment thread src/main/java/Yeos.java Outdated
}
scanner.close();
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is easy to read and appropriate indentations throughout the code. Good job!

Comment thread src/main/java/Yeos.java Outdated
}
System.out.println(line + input + "\n" + line);
else if (input.equals("list")) {
int task_counter = 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good if the variable name is in camelCase, as per coding standards

@yeozongyao yeozongyao changed the title Commits for Level-0 to Week 4 [Yeo Zong Yao] IP Feb 9, 2024
Comment thread src/main/java/Sinep.java Outdated
System.out.println(markingTask.getStatusIcon() + " " + markingTask.description + nl + line);
} else if (input.startsWith("unmark ")) {
int taskIndex = Integer.parseInt(input.substring(7)) - 1;
Task markingTask = taskList.get(taskIndex);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable names throughout code follows coding standards. Good job!

Comment thread src/main/java/Sinep.java Outdated
public class Sinep {
public static void main(String[] args) {
Scanner scanner = new Scanner(System.in);
String greeting = "Hello! I'm Sinep, your personal chatbot!\n"

@hongyijie06 hongyijie06 Feb 9, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation for wrapped lines is correct (8 spaces) 👍

Comment thread src/main/java/Sinep.java Outdated
System.out.println(line + nl + "Added to task list: " + nl + descriptionToPrint);
System.out.println("Now you have " + taskList.size() + " tasks in the list." + nl + line);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable names are appropriate and code is neat and easy to read :)

Comment thread src/main/java/Event.java Outdated

@Override
public String toString() {
return "[E]" + getStatusIcon() + " " + description + " (from: " + startDateTime + " to: " + endDateTime + ")";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation of the return statement may have been a little too much :0
Just 4 spaces would be good

@Jai2501 Jai2501 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Zong Yao!

Good job on the work done so far!

I have reviewed your code and have left some comments. Please go through them and get back to me for any clarifications.

Thanks!

Comment thread build.gradle

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on incorporating Gradle!

Comment on lines +1 to +6
package ExceptionHandling;


public class InvalidCommandException extends Exception{

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty Class. Please add at least a constructor with valid instantiations/assignments, so if this error is thrown, the description shown is correct. This has been observed in multiple files.

Else please add a comment explaining when would this error be thrown and why is it empty and nothing initialised.

Comment on lines +3 to +4
public class InvalidTaskIndexException extends Exception{
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Advisable to have a uniform spacing strategy, to enhance code readability.

Suggested change
public class InvalidTaskIndexException extends Exception{
}
public class InvalidTaskIndexException extends Exception {
}

Comment thread src/main/java/Sinep/Deadline.java Outdated
Comment on lines +16 to +18



Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may remove these extra lines. In IntelliJ, the code formatter can be updated to do this for you, every time you hit CTRL/CMD + S.

Comment thread src/main/java/Sinep/Deadline.java Outdated
Comment on lines +13 to +15
public String toString() {
return "[D]" + getStatusIcon() + " " + description + " (by: " + dateTime + ")";
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is usually a practise to use this.method() or this.variable when that method or variable belongs to the Class.

This change can be made at other places as well :)

Suggested change
public String toString() {
return "[D]" + getStatusIcon() + " " + description + " (by: " + dateTime + ")";
}
public String toString() {
return "[D]" + this.getStatusIcon() + " " + this.description + " (by: " + this.dateTime + ")";
}

Comment thread src/main/java/Sinep/Sinep.java Outdated
Comment on lines +22 to +89
public static void main(String[] args) throws IOException {
loadTaskFile(taskList);
Scanner scanner = new Scanner(System.in);
printGreeting();
while (true) {
String input = scanner.nextLine();
String command = input.split(" ", 2)[0]; // Get the first word of the input as the command

switch (command) {
case "bye":
printBye();
scanner.close();
return;
case "list":
printList();
break;
case "mark":
try {
markList(input);
} catch (InvalidTaskIndexException e){
System.out.println("The task you are trying to edit does not exist!" + nl + line);
}
break;
case "unmark":
try {
unmarkList(input);
} catch (InvalidTaskIndexException e){
System.out.println("The task you are trying to edit does not exist!" + nl + line);
}
break;
case "todo":
try {
addTodo(input);
} catch (InvalidCommandMessageException e) {
System.out.println("The todo command message is invalid" + nl + line);
}
break;
case "deadline":
try {
addDeadline(input);
} catch (InvalidCommandMessageException e) {
System.out.println("The deadline command is invalid" + nl +line);
}
break;
case "event":
try {
addEvent(input);
} catch (InvalidCommandMessageException e) {
System.out.println("The event command is invalid" + nl + line);
}
break;
case "delete":
try {
deleteTask(input);
} catch (InvalidTaskIndexException | InvalidCommandMessageException e) {
System.out.println("The delete command is invalid" + nl + line);
}
break;
default:
try {
throw new InvalidCommandException();
} catch (InvalidCommandException e) {
System.out.println("Please enter a valid command: todo, deadline or event");
}
}
saveTasks(taskList);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This code block can be abstracted to improve the code readability. Usually, methods should not be longer than 30 Lines of Code.
  2. There is deep nesting present in this block. You may need to work on abstraction for this as well.

Reference:
https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-long-methods
https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-deep-nesting

Comment thread src/main/java/Sinep/Sinep.java Outdated
Comment on lines +109 to +144
try {
Scanner scanner = new Scanner(taskFile);
int counter = 0;
while (scanner.hasNext()) {
String nextLine = scanner.nextLine();
if (nextLine.startsWith("[T]")) {
String description = nextLine.substring(6).trim();
taskList.add(new Todo(description));
if (nextLine.charAt(4) == 'X') {
Task markingTask = taskList.get(counter);
markingTask.markAsDone();
}
} else if (nextLine.startsWith("[D]")) {
String command = getDeadlineCommand(nextLine);
System.out.println(command);
taskList.add(new Deadline(command));
if (nextLine.charAt(4) == 'X') {
Task markingTask = taskList.get(counter);
markingTask.markAsDone();
}
} else {
String command = getEventCommand(nextLine);
taskList.add(new Event(command));
if (nextLine.charAt(4) == 'X') {
Task markingTask = taskList.get(counter);
markingTask.markAsDone();
}
}
counter++;
}
} catch (FileNotFoundException e) {
File parentDir = taskFile.getParentFile();
if (!parentDir.exists() && parentDir.mkdirs()) {
throw new IOException("File not found.");
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here, this is a deeply nested code. Please work on the abstraction.

Comment thread src/main/java/Sinep/Sinep.java Outdated
Comment on lines +147 to +163
private static String getEventCommand(String nextLine) {
int startDescription = nextLine.indexOf("[E]") + 6;
int endDescription = nextLine.indexOf(" (from:");
String description = nextLine.substring(startDescription, endDescription).trim();
String fromKeyword = "(from: ";
String toKeyword = " to:";
int start = nextLine.indexOf(fromKeyword) + fromKeyword.length();
int end = nextLine.indexOf(toKeyword);
String fromTime = nextLine.substring(start, end).trim();
String startKeyword = "to: ";
int start1 = nextLine.indexOf(startKeyword) + startKeyword.length();
int end1 = nextLine.lastIndexOf(')');
String toTime = nextLine.substring(start1, end1).trim();
String command;
command = "event " + description + " /from " + fromTime + " /to " + toTime;
return command;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is advisable to have logically linked code to be put together, and other part separated by a line break. Please make these changes everywhere you see fit.

Example: (Add line breaks as you deem right)

Suggested change
private static String getEventCommand(String nextLine) {
int startDescription = nextLine.indexOf("[E]") + 6;
int endDescription = nextLine.indexOf(" (from:");
String description = nextLine.substring(startDescription, endDescription).trim();
String fromKeyword = "(from: ";
String toKeyword = " to:";
int start = nextLine.indexOf(fromKeyword) + fromKeyword.length();
int end = nextLine.indexOf(toKeyword);
String fromTime = nextLine.substring(start, end).trim();
String startKeyword = "to: ";
int start1 = nextLine.indexOf(startKeyword) + startKeyword.length();
int end1 = nextLine.lastIndexOf(')');
String toTime = nextLine.substring(start1, end1).trim();
String command;
command = "event " + description + " /from " + fromTime + " /to " + toTime;
return command;
}
private static String getEventCommand(String nextLine) {
int startDescription = nextLine.indexOf("[E]") + 6;
int endDescription = nextLine.indexOf(" (from:");
String description = nextLine.substring(startDescription, endDescription).trim();
String fromKeyword = "(from: ";
String toKeyword = " to:";
int start = nextLine.indexOf(fromKeyword) + fromKeyword.length();
int end = nextLine.indexOf(toKeyword);
String fromTime = nextLine.substring(start, end).trim();
String startKeyword = "to: ";
int start1 = nextLine.indexOf(startKeyword) + startKeyword.length();
int end1 = nextLine.lastIndexOf(')');
String toTime = nextLine.substring(start1, end1).trim();
String command;
command = "event " + description + " /from " + fromTime + " /to " + toTime;
return command;
}

Reference: https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#structure-code-logically

Comment thread src/main/java/Sinep/Sinep.java Outdated
Comment on lines +188 to +211
protected static void saveTasks(ArrayList<Task> taskList) {
if (taskList.isEmpty()) {
try {
writeToFile("", false);
} catch (IOException e) {
System.out.println("Error saving your file. IO issues.");
}
} else {
try {
writeToFile(taskList.get(0).toString(), false);
} catch (IOException e) {
System.out.println("Error saving your file. IO issues.");
}
int i = 1;
while (i < taskList.size()) {
try {
writeToFile(taskList.get(i).toString(), true);
i += 1;
} catch (IOException e) {
System.out.println("Error saving your file. IO issues.");
}
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation of saveTasks handles the task list and file writing in a repetitive and somewhat inefficient manner. Consider abstracting the file writing logic into a separate method to reduce repetition and improve readability. Additionally, the exception handling could be streamlined to avoid duplicating the catch block.

Comment thread src/main/java/Sinep/Sinep.java Outdated
System.out.println(line + nl + "Added to task list:" + nl + descriptionToPrint);
System.out.println("Now you have " + taskList.size() + " tasks in the list." + nl + line);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I believe the Sinep class has a lot of methods, that are not necessarily related to it, and these may be abstracted to another Class such as TaskManager or TaskList.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants