Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Jwen/scoreboard real data per genre #264

Merged
merged 10 commits into from
May 9, 2022

Conversation

jiabaow
Copy link
Collaborator

@jiabaow jiabaow commented May 8, 2022

retrieve actual data from firebase to each scoreboard per genre, and sort them by descending order.

@jiabaow jiabaow linked an issue May 8, 2022 that may be closed by this pull request
R.id.rockButton-> {method = LastfmMethod.BY_TAG.method; tag = "rock"; mode = "rock" }
R.id.topTracksButton -> {method = LastfmMethod.BY_CHART.method; mode = "top tracks"}
R.id.billieEilishButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "Billie Eilish"; mode = "billie eilish"}
R.id.btsButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "BTS"; mode = R.string.bts.toString() }
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 5 locations. Consider refactoring.

R.id.topTracksButton -> {method = LastfmMethod.BY_CHART.method; mode = "top tracks"}
R.id.billieEilishButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "Billie Eilish"; mode = "billie eilish"}
R.id.btsButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "BTS"; mode = R.string.bts.toString() }
R.id.kpopButton -> {method = LastfmMethod.BY_TAG.method; tag = "kpop"; mode = R.string.kpop.toString() }
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 5 locations. Consider refactoring.

R.id.billieEilishButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "Billie Eilish"; mode = "billie eilish"}
R.id.btsButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "BTS"; mode = R.string.bts.toString() }
R.id.kpopButton -> {method = LastfmMethod.BY_TAG.method; tag = "kpop"; mode = R.string.kpop.toString() }
R.id.imagDragonsButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "Imagine Dragons"; mode = R.string.imagine_dragons.toString()}
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 5 locations. Consider refactoring.

R.id.btsButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "BTS"; mode = R.string.bts.toString() }
R.id.kpopButton -> {method = LastfmMethod.BY_TAG.method; tag = "kpop"; mode = R.string.kpop.toString() }
R.id.imagDragonsButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "Imagine Dragons"; mode = R.string.imagine_dragons.toString()}
R.id.rockButton-> {method = LastfmMethod.BY_TAG.method; tag = "rock"; mode = R.string.rock.toString() }
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 5 locations. Consider refactoring.

R.id.imagDragonsButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "Imagine Dragons"; mode = R.string.imagine_dragons.toString()}
R.id.rockButton-> {method = LastfmMethod.BY_TAG.method; tag = "rock"; mode = R.string.rock.toString() }
R.id.topTracksButton -> {method = LastfmMethod.BY_CHART.method; mode = R.string.top_tracks.toString()}
R.id.billieEilishButton -> {method = LastfmMethod.BY_ARTIST.method; artist = "Billie Eilish"; mode = R.string.billie_eilish.toString()}
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 5 locations. Consider refactoring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to refactor these lines in, for example, a function that takes the "method" and the "mode" as parameters and the artist/tag as an optional String parameter? It's not urgent but it could solve the detected duplication.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will keep this in mind when we do the general refactoring.

@codeclimate
Copy link

codeclimate bot commented May 8, 2022

Code Climate has analyzed commit d50f08b and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 5

The test coverage on the diff in this pull request is 93.9% (80% is the threshold).

This pull request will bring the total coverage in the repository to 82.1% (0.2% change).

View more on Code Climate.

@jiabaow jiabaow marked this pull request as ready for review May 8, 2022 13:25
@jiabaow jiabaow self-assigned this May 8, 2022
Copy link
Collaborator

@kamilababayeva kamilababayeva left a comment

Choose a reason for hiding this comment

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

Thank you for the changes! I have left minor comment, but it looks good to me!

app/src/main/java/ch/sdp/vibester/user/User.kt Outdated Show resolved Hide resolved
R.id.imagDragonsButton -> {sortedBy += R.string.imagine_dragons; genre = "Imagine Dragons"}
R.id.billieEilishButton -> {sortedBy += R.string.billie_eilish; genre = "Billie Eilish"}
R.id.rockButton -> {sortedBy += R.string.rock; genre = "rock"}
R.id.topTracksButton -> {sortedBy += R.string.top_tracks; genre = "top tracks"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we put genre into the strings as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried but that doesn't work, I explained in the comment in line 39.

Copy link
Collaborator

@zwierski zwierski left a comment

Choose a reason for hiding this comment

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

Very good changes! I left a question about the duplication issue but it's not something that needs to be changed urgently.

@jiabaow jiabaow merged commit 789e5b6 into main May 9, 2022
@jiabaow jiabaow deleted the jwen/scoreboard-real-data-per-genre branch May 9, 2022 07:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

retrieve actual data from firebase per playlist in scoreboard (#216)
3 participants