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

Quality of Life refactors #208

Merged
merged 6 commits into from
Apr 4, 2023

Conversation

shirsho-12
Copy link

No description provided.

Copy link

@nicleejy nicleejy left a comment

Choose a reason for hiding this comment

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

Clear refactoring of the codebase to follow OOP principles. Looks good to me overall!

Copy link

Choose a reason for hiding this comment

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

A good decision to let EditCommand be an interface which extends from Command, follows OOP principles more closely.


if (!internalListOfCategories.remove(toRemove)) {
if (!internalListOfCategories.remove(category)) {
//Throw an exception here later
Copy link

Choose a reason for hiding this comment

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

This part should be implemented by throwing an exception if the category to remove not being present in the list. If the Category was already checked to have been contained earlier, then this need not be an if statement.

if (!recurringExpense.expenseCategory.equals(this.expenseCategory)) {
return false;
}

if (!recurringExpense.expenseName.equals(this.expenseName)) {
return false;
}
Copy link

Choose a reason for hiding this comment

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

There could be a less repetitive way of doing this, can consider refactoring, but it is not a big issue.

Copy link
Author

Choose a reason for hiding this comment

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

i agree, i'll refactor it

@shirsho-12 shirsho-12 merged commit 228fbfd into AY2223S2-CS2103T-W09-2:master Apr 4, 2023
@shirsho-12 shirsho-12 deleted the uml-diagrams branch April 4, 2023 13:24
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.

2 participants