-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
val editUsername = findViewById<Button>(R.id.editUser) | ||
val editHandle = findViewById<Button>(R.id.editHandle) | ||
|
||
editUsername.setOnClickListener { |
There was a problem hiding this comment.
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.
showDialog("Change username", "Enter new username", 0, R.id.username) | ||
} | ||
|
||
editHandle.setOnClickListener { |
There was a problem hiding this comment.
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.
In this PR I implemented google sign in with the tests I could, added email and password validation and implemented username and handle change |
Code Climate has analyzed commit d03ad41 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 85.0% (80% is the threshold). This pull request will bring the total coverage in the repository to 93.5% (-1.5% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple warnings about using android resources and hardcoding strings. We already have quite a few things to fix and will fix these soon as well, however in the future please pay attention to remove as many of these as possible. Android studio's "Problems" tab at the bottom shows both errors and warnings, which might be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good code in overall 👍 , just missing some javadoc maybe 😅
|
||
private fun stringValidation(username: String, password: String): Boolean{ | ||
if(username.isEmpty() || password.isEmpty()) { | ||
email.text = "Empty email or password" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be using string.xml
@@ -16,7 +16,7 @@ import com.google.firebase.auth.AuthResult | |||
|
|||
class Register : AppCompatActivity() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May add javadoc to class and methods ! Maybe some comments too
// } | ||
// } | ||
|
||
fun googleActivityResult(requestCode: Int, resultCode: Int, data: Intent?): String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc ? javadoc ?
@@ -2,7 +2,7 @@ package ch.sdp.vibester.profile | |||
|
|||
class ProfileDataProvider(userID: String, users: List<UserProfile> = emptyList(), scoreboard: List<Int> = emptyList()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments but the rest LGTM
// } | ||
|
||
fun googleActivityResult(requestCode: Int, resultCode: Int, data: Intent?): String? { | ||
return if(requestCode == 1000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe replace this magic number by a more meaningful name
@@ -38,12 +38,14 @@ | |||
android:layout_marginTop="31dp" | |||
android:layout_marginBottom="19dp" | |||
android:ems="10" | |||
android:hint="email" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using @string and I feel like showing "TextView" directly on the screen is a bit wired.
fun derinTest() { | ||
val a = FireBaseAuthenticator() | ||
a.googleActivityResult(-1,-1,null) | ||
onView(withId(R.id.email)).check(matches(withText("TextView"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a "TextView" here, is it shown explicitly on the screen?
Good comments, I'll merge this and fix them with my next PR. I want to have google Sign In in our demo just in case I don't finish my other task |
No description provided.