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

Tags collation #76

Merged
merged 56 commits into from Oct 26, 2019
Merged

Tags collation #76

merged 56 commits into from Oct 26, 2019

Conversation

moziliar
Copy link

@moziliar moziliar commented Oct 23, 2019

Closes #81 Add tag collation feature and a visual view in the form of PieChart.

  • Build a TagManager that parses a given MemeBook for the tags.

  • Adds a PieChart display of tags in a given MemeBook.

Copy link

@jonchan51 jonchan51 left a comment

Choose a reason for hiding this comment

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

I'm ok with this implementation right now. Might want to work with zimu on some parts as he will definitely need a list of tags for his feature afterwards. So it might make more sense to have a TagManager outside of statistics instead.

src/main/java/seedu/weme/MainApp.java Outdated Show resolved Hide resolved
src/main/java/seedu/weme/statistics/TagManager.java Outdated Show resolved Hide resolved
src/main/java/seedu/weme/statistics/TagManager.java Outdated Show resolved Hide resolved
src/main/java/seedu/weme/statistics/TagManager.java Outdated Show resolved Hide resolved
Copy link

@BillChee123 BillChee123 left a comment

Choose a reason for hiding this comment

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

Looks good, just require some minor changes, mostly dealing with magic literals.

src/main/java/seedu/weme/statistics/TagManager.java Outdated Show resolved Hide resolved
src/main/java/seedu/weme/ui/StatsPanel.java Outdated Show resolved Hide resolved
src/main/java/seedu/weme/ui/StatsPanel.java Outdated Show resolved Hide resolved
src/main/java/seedu/weme/statistics/Stats.java Outdated Show resolved Hide resolved

//============= Tag Data ====================================

List<TagWithCount> getTagsWithCountList(ObservableList<Meme> memeList);

Choose a reason for hiding this comment

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

Suggested change
List<TagWithCount> getTagsWithCountList(ObservableList<Meme> memeList);
List<TagWithCount> getTagsWithCountList(List<Meme> memeList);

src/main/java/seedu/weme/statistics/TagManager.java Outdated Show resolved Hide resolved
/**
* Get {@code TagWithCount} in PriorityQueue from a {@code ReadOnlyMemeBook}.
*/
public PriorityQueue<TagWithCount> getTagsInOrderOfCounts(ObservableList<Meme> memeList) {

Choose a reason for hiding this comment

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

Suggested change
public PriorityQueue<TagWithCount> getTagsInOrderOfCounts(ObservableList<Meme> memeList) {
public PriorityQueue<TagWithCount> getTagsInOrderOfCounts(List<Meme> memeList) {

update comments accordingly

Copy link
Author

Choose a reason for hiding this comment

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

The method takes in an ObservableList of Memes.

Choose a reason for hiding this comment

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

Was thinking could just make it List instead since it doesn't necessarily have to be an observable list.

src/main/java/seedu/weme/statistics/TagManager.java Outdated Show resolved Hide resolved
@moziliar moziliar merged commit 46a7379 into master Oct 26, 2019
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.

Tag collation and view in pie chart
3 participants