Skip to content

[Wandan Linda Li] iP - #80

Open
wli-linda wants to merge 43 commits into
nus-cs2113-AY2122S2:masterfrom
wli-linda:master
Open

wli-linda wants to merge 43 commits into
nus-cs2113-AY2122S2:masterfrom
wli-linda:master

Conversation

@wli-linda

Copy link
Copy Markdown

No description provided.

Also, refactoring some code
Editing some things to match coding standard and adding customizations
kind of a messy implementation :'(
and some debugging / refactoring / changes in text
with regression testing; small change to output message
And some more debugging / refactoring

@quitejasper quitejasper 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.

Overall really impressed with your work. I believe you have put in a lot of effort in this. Well done!

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

@cheshire-doge cheshire-doge 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.

I think you did a great job following coding conventions! I could not spot any glaring issues, keep up the good work!

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

@xseh xseh 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.

Good job! Consider removing the magic literals and refactor for better code readability.

Comment thread src/main/java/duke/Duke.java Outdated
Comment thread src/main/java/duke/Duke.java Outdated
Comment thread src/main/java/duke/Duke.java Outdated
Comment thread src/main/java/duke/Duke.java Outdated
}
curr = list.get(taskNum);
} catch (IndexOutOfBoundsException | NumberFormatException e) {
// NumberFormatException caught in IntelliJ runtime env but not in testing...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider writing to the reader instead of private notes to yourself.

Comment thread src/main/java/duke/Duke.java Outdated
Comment on lines +107 to +109
String[] deadlineBreakdown = description.split("/by ", 2);
description = deadlineBreakdown[0];
by = deadlineBreakdown[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.

Consider refactoring code into a separate method since it is needed later.

Comment thread src/main/java/duke/Duke.java Outdated
Comment thread src/main/java/duke/Duke.java Outdated
Comment thread src/main/java/duke/ReadTaskList.java Outdated
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