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

implement SharedPref #150

Merged
merged 25 commits into from
Apr 7, 2022
Merged

implement SharedPref #150

merged 25 commits into from
Apr 7, 2022

Conversation

MaximeZmt
Copy link
Owner

@MaximeZmt MaximeZmt commented Apr 2, 2022

UserSharedPref

This is related to issue #142

This Pull Requests contains:

  • model.UserSharedPref: Static class that managed the shared preferences for the whole app, using User abstraction
  • activity.WelcomeActivity: Small update with the text indicating if logged in or not
  • database.UserRepo: Adding a method to update int field in db.

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

Notes for reviewers

  • Big update of ProfileActivityTest
  • Currently at the end of game, update only +1 on number of game but other stat will be done in future
  • Still hardcoded a lot, has to be connected to Account creation

}
}

fun updateScore(ctx: Context, deltaTotal: Int = 0, deltaBest: Int = 0, deltaCorrect: Int = 0, deltaRanking: Int = 0){
Copy link

Choose a reason for hiding this comment

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

Method updateScore has 5 arguments (exceeds 4 allowed). Consider refactoring.

}


fun updateUsername(ctx: Context, username: String){
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.

}
}

fun updateHandle(ctx: Context, handle: String){
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 April 7, 2022 20:32
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.

Just couple of very small things but overall good work! LGTM

* @param newVal (Int) the new value of the field that is being updated
* @param fieldName the field name of the field that is being updated
*/
fun updateFieldInt(userID: String, newVal: Int, fieldName: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can pass newVal as a generic type and have one function? What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like in this example
image

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think that might be a good idea for a future refactor. Like this is not important yet would be done to improve maintainability.

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.

Good job! Everything looks good to me.

}
}

/**
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

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.

@codeclimate
Copy link

codeclimate bot commented Apr 7, 2022

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

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 4

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

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

View more on Code Climate.

@MaximeZmt MaximeZmt merged commit 8ed5feb into main Apr 7, 2022
@MaximeZmt MaximeZmt deleted the maximezmt/profileWholeApp branch April 7, 2022 23:43
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.

Passes the profile, into the different activity, show the username into the welcomescreen (#141)
3 participants