Add logic for offline game - PART 2 #349
Conversation
MaximeZmt
changed the title
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| goneView = requireView().findViewById<LinearLayout>(R.id.chooseGame), | ||
| visibleView = requireView().findViewById<ConstraintLayout>(R.id.chooseSetting) | ||
| ) | ||
| } else{ |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| visibleView = requireView().findViewById<ConstraintLayout>(R.id.genrePerScoreboard)) | ||
| if (playOffline) { | ||
| chooseGenre(method = LastfmMethod.BY_ARTIST.method, artist = "Personalized", mode = R.string.gameGenre_byArtistSearch, playOffline = playOffline) | ||
| } else { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| @@ -216,22 +221,30 @@ class GameSetupFragment : Fragment(), View.OnClickListener, AdapterView.OnItemSe | |||
| * @param tag: tag (genre) to fetch songs from: used in BY_TAG method | |||
| * @param mode: official game mode name | |||
| */ | |||
| private fun chooseGenre(method: String = "", artist: String = "", tag: String = "", mode: Int = 0) { | |||
| private fun chooseGenre(method: String = "", artist: String = "", tag: String = "", mode: Int = 0, playOffline: Boolean = false) { | |||
There was a problem hiding this comment.
Method chooseGenre has 5 arguments (exceeds 4 allowed). Consider refactoring.
There was a problem hiding this comment.
Would it be more useful to make playOffline a private field with a setter function to avoid this refactoring issue?
|
Code Climate has analyzed commit 1efcd80 and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 70.5% (80% is the threshold). This pull request will bring the total coverage in the repository to 84.3% (-0.9% change). View more on Code Climate. |
| goneView = requireView().findViewById<LinearLayout>(R.id.genrePerScoreboard), | ||
| visibleView = requireView().findViewById<ConstraintLayout>(R.id.chooseSetting) |
There was a problem hiding this comment.
For the CodeClimate issue, we can make the toggleViewsVisibility function take just a single boolean if the two arguments are always the same but switched around(i.e instead of tVV(a, b) you call tVV(b, a)) so the duplication can be fixed. Not an urgent thing, but could be useful if we want to refactor in the future
| if (playOffline) { | ||
| toggleViewsVisibility( | ||
| goneView = requireView().findViewById<LinearLayout>(R.id.chooseGame), | ||
| visibleView = requireView().findViewById<ConstraintLayout>(R.id.chooseSetting) |
| toggleViewsVisibility(goneView = vmGameSetup.view.findViewById<LinearLayout>(R.id.chooseGame), | ||
| visibleView = vmGameSetup.view.findViewById<ConstraintLayout>(R.id.genrePerScoreboard)) | ||
| if (playOffline) { | ||
| chooseGenre(method = LastfmMethod.BY_ARTIST.method, artist = "Personalized", mode = R.string.gameGenre_byArtistSearch, playOffline = playOffline) |
The new test coverage is a bit low, but it does not affect so much the overall coverage. So I will merge it like that and add tests if I have time and if it does not break ci.