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

Refactor #100

Merged
merged 15 commits into from
Mar 21, 2022
Merged

Refactor #100

merged 15 commits into from
Mar 21, 2022

Conversation

MaximeZmt
Copy link
Owner

@MaximeZmt MaximeZmt commented Mar 18, 2022

Refactor

This is related to issue #88

This Pull Requests contains:

  • Deletion of the GreetingsActivity (was created during bootcamp)

  • Create a Logo for the app (still a draft)

  • Choose a color palette

  • ReDesign from the welcome screen

  • Refactor the whole directory of the app, changes names of file with a better name

  • Has fixed 13 of 20 Maintainability issues

  • Improve readability of TypingGame (could still be improved)

  • removed MusicTemporary (not needed anymore)

  • Optimize imports for all app files

In addition the code is tested with a coverage of: [91.4]%

Notes for reviewers

  • WARNING the directory tree has been changed !!!

@MaximeZmt MaximeZmt self-assigned this Mar 18, 2022
@MaximeZmt MaximeZmt linked an issue Mar 18, 2022 that may be closed by this pull request
@MaximeZmt MaximeZmt changed the title deleting greeting activity Refactor Mar 19, 2022
private fun setupProfile(user: UserProfile){
findViewById<TextView>(R.id.handle).text = user.handle
findViewById<TextView>(R.id.username).text = user.username
findViewById<TextView>(R.id.totalGames).text = user.totalGames.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 4 locations. Consider refactoring.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Has not been changed, because this is temporary for setting manually the values

findViewById<TextView>(R.id.handle).text = user.handle
findViewById<TextView>(R.id.username).text = user.username
findViewById<TextView>(R.id.totalGames).text = user.totalGames.toString()
findViewById<TextView>(R.id.correctSongs).text = user.correctSongs.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 4 locations. Consider refactoring.

findViewById<TextView>(R.id.username).text = user.username
findViewById<TextView>(R.id.totalGames).text = user.totalGames.toString()
findViewById<TextView>(R.id.correctSongs).text = user.correctSongs.toString()
findViewById<TextView>(R.id.bestScore).text = user.bestScore.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 4 locations. Consider refactoring.

findViewById<TextView>(R.id.totalGames).text = user.totalGames.toString()
findViewById<TextView>(R.id.correctSongs).text = user.correctSongs.toString()
findViewById<TextView>(R.id.bestScore).text = user.bestScore.toString()
findViewById<TextView>(R.id.ranking).text = user.ranking.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 4 locations. Consider refactoring.

try {
val list = Song.listSong(task.await())
for (x: Song in list) {
if (mysong != null) {
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 2 locations. Consider refactoring.

Copy link
Owner Author

Choose a reason for hiding this comment

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

not duplication anymore, 2nd copy has been removed

mysong,
mediaPlayer
)
} else {
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 2 locations. Consider refactoring.

@MaximeZmt MaximeZmt marked this pull request as ready for review March 21, 2022 09:34
Copy link
Collaborator

@Tsathogguaa Tsathogguaa left a comment

Choose a reason for hiding this comment

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

genre/music/lyric temporary files have hardcoded strings, but can be changed on a different PR if necessary. Otherwise all good!

@MaximeZmt
Copy link
Owner Author

MaximeZmt commented Mar 21, 2022

genre/music/lyric temporary files have hardcoded strings, but can be changed on a different PR if necessary. Otherwise all good!

I didn't update it because it's a TemporaryFile. Thanks for review 👍

@kamilababayeva
Copy link
Collaborator

Since all of our tests are in Android Test, I would move ProfileDataProviderTest to Android Test folder. Also ProfileActivity is of different type than other files in activity folder, can you change that as well? I have attached the screenshot below.
Otherwise, the refactor looks good to me.

P.S. I am not sure why some test coverage had changed...
image

@MaximeZmt
Copy link
Owner Author

Since all of our tests are in Android Test, I would move ProfileDataProviderTest to Android Test folder. Also ProfileActivity is of different type than other files in activity folder, can you change that as well? I have attached the screenshot below. Otherwise, the refactor looks good to me.

P.S. I am not sure why some test coverage had changed... image

I didn't see the hidden test in the other folder, I will change that.

Concerning the Profile Activity, I've seen that problem of type, but I don't know why, gonna try again

The global coverage is going down by 0.2 because I've removed temporary files that were well tested. I've tried to improve one or two test, but the global tradeoff is still of 0.2

@MaximeZmt
Copy link
Owner Author

Since all of our tests are in Android Test, I would move ProfileDataProviderTest to Android Test folder. Also ProfileActivity is of different type than other files in activity folder, can you change that as well? I have attached the screenshot below. Otherwise, the refactor looks good to me.
P.S. I am not sure why some test coverage had changed... image

I didn't see the hidden test in the other folder, I will change that.

Concerning the Profile Activity, I've seen that problem of type, but I don't know why, gonna try again

The global coverage is going down by 0.2 because I've removed temporary files that were well tested. I've tried to improve one or two test, but the global tradeoff is still of 0.2

Oh I found the reason for the class problem, will be solved too

@codeclimate
Copy link

codeclimate bot commented Mar 21, 2022

Code Climate has analyzed commit 8c8c726 and detected 7 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3
Duplication 4

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

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

View more on Code Climate.

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 refactoring! The code looks much nicer!

@kamilababayeva kamilababayeva merged commit 034e0a0 into main Mar 21, 2022
@kamilababayeva kamilababayeva deleted the maximezmt/refactor branch March 21, 2022 11:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doing code refactor (#95)
3 participants