Skip to content
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

[Brandon Owen Sjarif] iP #82

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Brandon-OS
Copy link

No description provided.

Copy link

@xseh xseh left a comment

Choose a reason for hiding this comment

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

Good attempt! Do take some care to follow the coding standard and consider more SLAP to make the code more understandable.

Duke.java Outdated
import java.util.Scanner;

public class Duke {
// static variables
Copy link

Choose a reason for hiding this comment

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

Avoid repeating the obvious, consider removing the comment as the code is self-explanatory.

Duke.java Outdated

public class Duke {
// static variables
private static Task[] taskList = new Task[100];
Copy link

Choose a reason for hiding this comment

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

This code violates the coding standards. As mentioned on the module website:

Plural form should be used on names representing a collection of objects.

As such, perhaps you can consider using a more appropriate name (e.g. tasks) to comply with the coding standards.

Duke.java Outdated
greet();
Scanner input = new Scanner(System.in);
String line = input.nextLine();
String command = line.split(" ")[0];
Copy link

Choose a reason for hiding this comment

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

Avoid magic numbers. Perhaps putting the number into a descriptive variable would be more suitable. For example, 0 can be added into the variable COMMAND_INDEX to make the code more understandable.

Duke.java Outdated

while (!command.matches("bye")) {
switch (command) {
case "list":
Copy link

Choose a reason for hiding this comment

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

Avoid magic strings. Similiarly, consider adding the strings into a descriptive variable to improve code readablility. For example, list can be added into the variable COMMAND_WORD_LIST.

Duke.java Outdated
Comment on lines 44 to 45
int index = Integer.parseInt(line.split(" ")[1]) - 1;
markTask(taskList[index]);
Copy link

Choose a reason for hiding this comment

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

These two statements have different levels of abstraction so consider further extracting the logic (SLAP more) into another method to make the code more readable.

Duke.java Outdated
Comment on lines 60 to 61


Copy link

Choose a reason for hiding this comment

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

Be careful of spaces that violates the coding standards. You might want to perhaps consider removing empty spaces to make the code more readable.

Duke.java Outdated
Comment on lines 134 to 185
public static void addTaskSpecial(String name, String type) {
String divider = "/";
if (!checkSpace(name)) {
errorMessage();
return;
}

if (!name.contains(divider)) {
System.out.println(HORIZONTAL_LINE);
System.out.println("Please add a date");
System.out.println(HORIZONTAL_LINE);
greet();
return;
}

name = name.substring(name.indexOf(" "), name.length());
switch (type) {
case "todo":
Todo todoTask = new Todo(name);
taskList[taskCounter] = todoTask;
taskCounter += 1;
System.out.println(HORIZONTAL_LINE);
System.out.println("Got it. I've added this task:");
System.out.println(todoTask.toString());
System.out.println("Now you have " + taskCounter + " tasks in the list.");
System.out.println(HORIZONTAL_LINE);
break;
case "deadline":
String deadlineDate = name.substring(name.indexOf(divider) + 3, name.length());
Deadline deadlineTask = new Deadline(name, deadlineDate);
taskList[taskCounter] = deadlineTask;
taskCounter += 1;
System.out.println(HORIZONTAL_LINE);
System.out.println("Got it. I've added this task:");
System.out.println(deadlineTask.toString());
System.out.println("Now you have " + taskCounter + " tasks in the list.");
System.out.println(HORIZONTAL_LINE);
break;
case "event":
String eventDate = name.substring(name.indexOf(divider) + 3, name.length());
Event eventTask = new Event(name, eventDate);
taskList[taskCounter] = eventTask;
taskCounter += 1;
System.out.println(HORIZONTAL_LINE);
System.out.println("Got it. I've added this task:");
System.out.println(eventTask.toString());
System.out.println("Now you have " + taskCounter + " tasks in the list.");
System.out.println(HORIZONTAL_LINE);
break;
}
}
}
Copy link

Choose a reason for hiding this comment

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

Avoid long methods. As mentioned on the module website:

Be wary when a method is longer than the computer screen, and take corrective action when it goes beyond 30 LOC (lines of code). The bigger the haystack, the harder it is to find a needle.

Therefore, you might want to consider putting some of the logic into seperate methods (SLAP more) to reduce the method length and make the code more understandable.

Duke.java Outdated
System.out.println(HORIZONTAL_LINE);
break;
case "event":
String eventDate = name.substring(name.indexOf(divider) + 3, name.length());
Copy link

Choose a reason for hiding this comment

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

Avoid complicated expressions. Consider breaking down the logic into seperate steps to make the code more understandable.

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.

2 participants