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

Create RestaurantSummary and display it in the GUI #103

Merged

Conversation

weixin-koh
Copy link
Collaborator

Closes #90

@weixin-koh weixin-koh added this to the v1.3 milestone Apr 2, 2019
@weixin-koh weixin-koh added priority.Medium Nice to have type.Enhancement An enhancement to an existing story status.Ongoing The issue/PR is currently being worked on. note: remove this label before closing an issue. severity.Low A flaw that is unlikely to affect normal operations of the product. Appears only in very rare situat labels Apr 2, 2019
Copy link
Collaborator

@rbth7e5 rbth7e5 left a comment

Choose a reason for hiding this comment

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

Remember to write tests for the new classes implemented. Especially the Summary class.

@@ -107,6 +111,7 @@ public Restaurant(Restaurant restaurant, Category category) {
this.categories = category;
this.weblink = restaurant.weblink;
this.openingHours = restaurant.openingHours;
this.summary = new Summary(restaurant.getReviews());
}

public Name getName() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using getters, would be better to create a method. For example, it is possible to pass the UI components into a method to set the name values and summary data.

@weixin-koh weixin-koh added the status.ReadyForReview The PR is ready for review label Apr 2, 2019
@weixin-koh weixin-koh requested a review from Wklee96 April 2, 2019 14:02
.append(" visits");
return builder.toString();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good Work! Since it is a new class, perhaps you can write a summary class test to ensure it is working perfectly!

@weixin-koh weixin-koh merged commit ce8c66d into CS2103-AY1819S2-W17-1:master Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.Medium Nice to have severity.Low A flaw that is unlikely to affect normal operations of the product. Appears only in very rare situat status.Ongoing The issue/PR is currently being worked on. note: remove this label before closing an issue. status.ReadyForReview The PR is ready for review type.Enhancement An enhancement to an existing story
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants