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

Refactor of intent fireing + LyricsBelongGameTest #268

Merged
merged 16 commits into from
May 11, 2022

Conversation

MaximeZmt
Copy link
Owner

@MaximeZmt MaximeZmt commented May 9, 2022

Refactor of intent fireing + LyricsBelongGameTest

@MaximeZmt MaximeZmt self-assigned this May 9, 2022
@MaximeZmt MaximeZmt linked an issue May 9, 2022 that may be closed by this pull request
@MaximeZmt MaximeZmt added the actual: 3h actual time to fix label May 9, 2022
@@ -55,6 +56,10 @@ class DownloadActivity : AppCompatActivity() {
downloadListener(songNameView)
}

findViewById<FloatingActionButton>(R.id.download_returnToMain).setOnClickListener {
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 3 locations. Consider refactoring.

@@ -32,6 +33,10 @@ class DeleteSongsActivity : AppCompatActivity() {

setContentView(R.layout.activity_delete_songs)

findViewById<FloatingActionButton>(R.id.delete_returnToMain).setOnClickListener {
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 3 locations. Consider refactoring.

@@ -30,6 +31,11 @@ class IncorrectSongsActivity : AppCompatActivity() {

val layout: LinearLayout = findViewById(R.id.incorrect_songs_linear)

findViewById<Button>(R.id.incorrect_songs_back_to_welcome)
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 3 locations. Consider refactoring.

@MaximeZmt MaximeZmt marked this pull request as ready for review May 11, 2022 17:45
@MaximeZmt MaximeZmt requested a review from kamilababayeva May 11, 2022 17:47
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.

I have some questions regarding testing some lines!

Thank you for your changes! Looks good to me

if(this::currentSong.isInitialized) {
return currentSong
} else {
return Song.songBuilder("http://example.com", "http://example.com", songName, artistName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the purpose if this song.builder?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The purpose of this song builder is that when the currentSong is not initialized yet (mean no web connection for instance/low connection), it does not crash and has song title and name

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure whether we should put a fake song. maybe we can put some text to screen. lets fix it next pr!

Copy link
Owner Author

Choose a reason for hiding this comment

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

this was not a fakesong. Just a fake url, but title and artist name is truth.
Indeed there is maybe another solution.

app/src/main/java/ch/sdp/vibester/helper/IntentSwitcher.kt Outdated Show resolved Hide resolved
Copy link
Collaborator

@laurislopata laurislopata left a comment

Choose a reason for hiding this comment

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

LGTM, just a tiny comment but I think you can add it later if you want :)

@kamilababayeva kamilababayeva self-requested a review May 11, 2022 20:17
@MaximeZmt MaximeZmt added actual: 7h and removed actual: 3h actual time to fix labels May 11, 2022
@codeclimate
Copy link

codeclimate bot commented May 11, 2022

Code Climate has analyzed commit ccc44b5 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 3

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

This pull request will bring the total coverage in the repository to 82.1%.

View more on Code Climate.

@MaximeZmt MaximeZmt merged commit 0a14ea4 into main May 11, 2022
@MaximeZmt MaximeZmt deleted the maximezmt/fireIntentRefactor branch May 11, 2022 20:52
@MaximeZmt MaximeZmt changed the title some refactor of intent fireing Refactor of intent fireing + LyricsBelongGameTest May 11, 2022
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.

Refactor of intent fireing + LyricsBelongGameTest
3 participants