Skip to content

[Aiman Imtiaz] iP#74

Open
aiman-imtiaz wants to merge 29 commits into
nus-cs2113-AY2122S2:masterfrom
aiman-imtiaz:master
Open

[Aiman Imtiaz] iP#74
aiman-imtiaz wants to merge 29 commits into
nus-cs2113-AY2122S2:masterfrom
aiman-imtiaz:master

Conversation

@aiman-imtiaz

Copy link
Copy Markdown

No description provided.

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

public static void unMarkTask(int index){

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 think unmark is a word and a verb so you might want to change the name to unmarkedTask

Comment thread src/main/java/Duke.java Outdated
static int currentCount = 0;
static String dashedLine = "\t____________________________________________________________";

public static void markTask(int index){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

to be more grammatically correct you may change the name of the method to markedTask

Comment thread src/main/java/Duke.java Outdated
static Task[] taskList = new Task[100];
static Boolean[] taskStatusList = new Boolean [100];
static int currentCount = 0;
static String dashedLine = "\t____________________________________________________________";

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 think constants should be named using all cap and there should be a final before to mark as constant.

Comment thread src/main/java/Duke.java Outdated
} else if(line.startsWith("unmark")){
int indexToUnmark = Integer.parseInt(line.substring(7));
unMarkTask(indexToUnmark);
} else addToList(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.

For if else even though it is just a line it is still better to put the content to a separate line and have curly brackets wrapped around it.

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

line = in.nextLine();

while (!line.contains("bye")){

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 how the main code to run here are very simple to read and I can really tell it is the main code.

@1szheng 1szheng 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.

Suggested some changes

Comment thread src/main/java/Duke.java Outdated
static int currentCount = 0;
static String dashedLine = "\t____________________________________________________________";

public static void markTask(int index){

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 could add spaces between the methods and the braces to be more consistent with the K&R style

Comment thread src/main/java/Duke.java Outdated
Comment on lines +75 to +86
while (!line.contains("bye")){
if (line.equals("list")){
printList();
} else if (line.startsWith("mark")){
int indexToMark = Integer.parseInt(line.substring(5));
markTask(indexToMark);
} else if(line.startsWith("unmark")){
int indexToUnmark = Integer.parseInt(line.substring(7));
unMarkTask(indexToUnmark);
} else addToList(line);
line = in.nextLine();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps we can use a switch-case instead?

Comment thread src/main/java/Duke.java Outdated
Comment on lines +5 to +6
static Task[] taskList = new Task[100];
static Boolean[] taskStatusList = new Boolean [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.

We can put the magic numbers as a constant instead

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

public static void printList(){
System.out.print(dashedLine);
for (int j = 0; j < currentCount; j++){

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 feel that the first loop counter should be i instead of j

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