Skip to content

[Alvin Ng] Duke Increments#363

Open
Alvinnyk wants to merge 69 commits intonus-cs2103-AY1920S1:masterfrom
Alvinnyk:master
Open

[Alvin Ng] Duke Increments#363
Alvinnyk wants to merge 69 commits intonus-cs2103-AY1920S1:masterfrom
Alvinnyk:master

Conversation

@Alvinnyk
Copy link
Copy Markdown

@Alvinnyk Alvinnyk commented Sep 3, 2019

No description provided.

@YiJiee
Copy link
Copy Markdown

YiJiee commented Sep 4, 2019

Packages are well organised into 3 components - Storage, Ui and Model.
Naming conventions for class file is a little bit inconsistent (some classes start with smaller case and others start with capital case).
Code is written clearly and easy to understand.
Specific exceptions were not implemented and exceptions were caught at the highest level (Exception).
Other than that, the app compiles and works great until Level-9.
Reviewed on 4 Sep 2019

@R-D-D-D
Copy link
Copy Markdown

R-D-D-D commented Sep 4, 2019

The TaskList extends ArrayList which saves so much trouble I should learn from you.
I think generally the run is too bulky, you might consider abstracting out the command and maybe a parser to parse user inputs.
Instead of representing a deadline task as a whole string, maybe can have a Date object to represent the date.
The rest looks good!

@iskandarzulkarnaien
Copy link
Copy Markdown

Code looks good! Some parts have heavily nested if-else blocks which might be worth refactoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants