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

[John Toh] iP #62

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

[John Toh] iP #62

wants to merge 31 commits into from

Conversation

jjtoh
Copy link

@jjtoh jjtoh commented Aug 29, 2022

No description provided.

jjtoh added 10 commits August 19, 2022 01:18
Improved code formatting
Duke can now add tasks and mark/unmark them
Edited some code to align it with the given stardards
Coding quality may need further improvement.
Has issues running the tests
Testing still has issues

@Override
public String toString() {
return "[E]" + super.getStatusIcon()+ " " + super.getDescription() + " (at: " + at + ")";

Choose a reason for hiding this comment

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

consider adding space after getStatusIcon().

Copy link

@Franky4566 Franky4566 left a comment

Choose a reason for hiding this comment

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

Well written code with good breakdowns. Just some minor issues and issue of SLAP.

}

public static void handleTodo(String input) {
String[] splitInput = input.split(" ", 2);

Choose a reason for hiding this comment

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

Consider naming 2 as a constant, difficult to tell what the number 2 means

private static final String MARK = "mark";
private static final String UNMARK = "unmark";

public static void handleInput() {

Choose a reason for hiding this comment

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

Consider adding comments for all ur functions so it is more readable, as described in the code quality page on the module website

Comment on lines 17 to 21
System.out.println("Marked this task: \n " + tasks[target - 1]);
} else {
System.out.println("Unmarked this task: \n" + tasks[target - 1]);
}
System.out.println(LINE_DIVIDER);

Choose a reason for hiding this comment

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

Phrases "Marked this task: \n" and "Unmarked this task: \n" can be written as a constant String when outputting
like line 21 declared in line 5

Comment on lines 32 to 35
String[] splitInput = input.split(" ", 2);
String[] splitCommand = splitInput[1].split(" /by ", 2); // 0 is description, 1 is by
Deadline deadline = new Deadline(splitCommand[0], splitCommand[1]);
TaskList.addDeadLine(deadline);

Choose a reason for hiding this comment

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

lines 32 and 33 can be abstracted into another function as the splitting is quite complicated (SLAP issue). Once extracted can be declared like line 35

Copy link

@exetr exetr left a comment

Choose a reason for hiding this comment

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

Great job on your code so far! Good to see a effective use of packaging within your application. The few comments I have left aims to improve the consistency in certain aspects of your code base.

Comment on lines 10 to 12
UI.welcomeUser();
FileManager.startReading();
InputHandler.handleInput();
Copy link

Choose a reason for hiding this comment

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

You should set the indentation to 4 spaces to ensure consistency and in adherance to the coding standard.


public class FileManager {

private static Path taskDataPath = Paths.get(System.getProperty("user.dir"), "data", "tasks.txt");
Copy link

Choose a reason for hiding this comment

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

Maybe consider replacing the tasks.txt portion with a constant, to eliminate magic literals.

File f = new File(filePath);
Scanner s = new Scanner(f);
while (s.hasNext()) {
String[] input = s.nextLine().split(" | ");
Copy link

Choose a reason for hiding this comment

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

Similarly, consider setting the delimiter you use as a constant, in order to eliminate magic literals.

Comment on lines 8 to 15
private static final String TODO = "todo";
private static final String LIST = "list";
private static final String DEADLINE = "deadline";
private static final String EVENT = "event";
private static final String BYE = "bye";
private static final String MARK = "mark";
private static final String UNMARK = "unmark";
private static final String DELETE = "delete";
Copy link

Choose a reason for hiding this comment

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

Great usage of final variables!

private static final String DELETE = "delete";

public static void handleInput() {
boolean exit = false;
Copy link

Choose a reason for hiding this comment

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

Consider renaming the boolean to isExit, in adherence to the naming convention.

Comment on lines 82 to 88
private static String getCommand(String input) {
return input.split(" ", 2)[0].trim();
}

private static String getAction(String input) throws ArrayIndexOutOfBoundsException {
return input.split(" ", 2)[1].trim();
}
Copy link

Choose a reason for hiding this comment

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

You could instead set the delimiter as a constant to eliminate magic literals.

System.out.println(LINE_DIVIDER);
}

private static void printMark(int target, boolean mark) {
Copy link

Choose a reason for hiding this comment

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

Consider renaming the mark variable to isMarked, to keep consistency in naming convention.

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