Skip to content

iP - #72

Draft
tuan0369 wants to merge 50 commits into
nus-cs2113-AY2122S2:masterfrom
tuan0369:add-intro
Draft

iP#72
tuan0369 wants to merge 50 commits into
nus-cs2113-AY2122S2:masterfrom
tuan0369:add-intro

Conversation

@tuan0369

@tuan0369 tuan0369 commented Feb 2, 2022

Copy link
Copy Markdown

Do everything from week 1 to week 4

Comment thread src/main/java/CommandType.java Outdated
@@ -0,0 +1,3 @@
public enum CommandType {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i like the use of enum

Comment thread src/main/java/Duke.java Outdated
System.out.println("───────────────────────────────────────────────────────────────────────");
}

public static void greeting() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can be named better. printGreeting

Comment thread src/main/java/Duke.java Outdated
Scanner input = new Scanner(System.in);
String line;

Todo[] tasks = new Todo[100];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

magic number 100, can be put as constant instead

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

switch (command) {
case TODO:
tasks[itemCount] = new Todo(line.substring(4));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

4,8,5,7, magic numbers!

Comment thread src/main/java/Todo.java Outdated
@Override
public String toString() {
return typeOfTask + taskStatus + " " + task;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no code violation. Good job!

Comment thread src/main/java/Event.java Outdated
@@ -0,0 +1,13 @@
public class Event extends Todo{
protected String at;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what is this variable representing? Maybe you can have a more descriptive name for the variable

Comment thread src/main/java/Duke.java Outdated
Scanner input = new Scanner(System.in);
String line;

Todo[] tasks = new Todo[100];

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 can create a constant variable for the number 100 since it wont be changed

Comment thread src/main/java/Duke.java Outdated
break;
case UNMARK:
int unmarkedItem = Integer.parseInt(line.substring(7)) - 1;
tasks[unmarkedItem].unmark();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

try not to have so many hard coded and same numbers in the switch statement

Comment thread src/main/java/CommandType.java Outdated
@@ -0,0 +1,3 @@
public enum CommandType {
TODO, DEADLINE, EVENT, MARK, UNMARK, LIST, NIL

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job on the use of enum

package util.miscellaneous;

public interface Chatbot {
String LOGO = " ____ _ \n"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job here with declaring constants, and avoiding magic numbers later!

Comment thread src/main/java/Duke.java Outdated
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);
Scanner input2 = new Scanner(System.in);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If this is input2, is there an input1? Consider a different name to explain and distinguish between variables.

String IO_ERROR_MSG = " Oh no! File IO error just occurred.";
String NO_PREVIOUS_RECORD = " There is currently no save file.";


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Additional blank lines could be removed to increase code quality and consistency.

System.out.println("\t" + GREETING_MSG_01);
System.out.println("\t" + GREETING_MSG_02);
linePrinter();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are blank lines like these before the closed curly brackets, yet other methods do not have this empty line. You could make the layout more consistent.

}

public static CommandType findCommandType(String line) {
CommandType c;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instead of naming with c, you could consider renaming as a noun for better code quality.

case MARK:
int markedItem = Integer.parseInt(line.substring(MARKED_ITEM_INDEX)) - 1;

if ((markedItem < 0) || (markedItem >= tasks.size())) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This compound boolean expression could be defined in intermediate steps.

}
}

public static void loadAndRun(ArrayList<Task> tasks, String line, boolean isLoadingData, boolean needUpdateTaskStatus) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This method is too long (>30 LoC). How about apply SLAP here to increase code readability?

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.

3 participants