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

Laurislopata/signin #52

Merged
merged 25 commits into from
Mar 13, 2022
Merged

Laurislopata/signin #52

merged 25 commits into from
Mar 13, 2022

Conversation

laurislopata
Copy link
Collaborator

No description provided.

startActivityForResult(intent, 1000)
}

private fun createAccount(email: String, password: String, emailText: TextView) {
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.

}
}

private fun signIn(email: String, password: String, emailText: TextView) {
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.

return auth.signInWithEmailAndPassword(email, password)
}

override fun createAccount(email: String, password: String): Task<AuthResult> {
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.

TODO("Not yet implemented")
}

override fun signIn(email: String, password: String): Task<AuthResult> {
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.

val password = findViewById<EditText>(R.id.password)
val email = findViewById<TextView>(R.id.email)

btCreateAcc.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 2 locations. Consider refactoring.

createAccount(username.text.toString(), password.text.toString(), email)
}

btLogIn.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 2 locations. Consider refactoring.

val auth: FirebaseAuth = Firebase.auth


fun signIn(email: String, password: String): Task<AuthResult> {
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.

return auth.signInWithEmailAndPassword(email, password)
}

fun createAccount(email: String, password: String): Task<AuthResult> {
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.

startActivityForResult(intent, 1000)
}

private fun createAccount(email: String, password: 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.

}
}

private fun signIn(email: String, password: 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.

@laurislopata laurislopata marked this pull request as ready for review March 13, 2022 13:26
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.

CodeClimate remarks refactoring but I see different functionalities for these functions. Some refactoring could be done nevertheless, but looks good already to me.

Copy link
Collaborator

@jiabaow jiabaow left a comment

Choose a reason for hiding this comment

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

LGTM
Don't forget to use @string for strings and resolve conflicts before merging

@@ -50,6 +50,7 @@ dependencies {
implementation 'androidx.appcompat:appcompat:1.4.1'
implementation 'com.google.android.material:material:1.5.0'
implementation 'androidx.constraintlayout:constraintlayout:2.1.3'
implementation 'com.google.firebase:firebase-auth-ktx:21.0.1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
implementation 'com.google.firebase:firebase-auth-ktx:21.0.1'
implementation 'com.google.firebase:firebase-auth-ktx:21.0.2'

super.onCreate(savedInstanceState)
setContentView(R.layout.activity_google_log_in)

// val gso = GoogleSignInOptions.Builder(GoogleSignInOptions.DEFAULT_SIGN_IN)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding @fixme or explain in the comment why stop using google sign in to remind us later in case

android:layout_width="200dp"
android:layout_height="48dp"
android:layout_marginTop="126dp"
android:text="Create account"
Copy link
Collaborator

Choose a reason for hiding this comment

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

use @string instead of hard code

@codeclimate
Copy link

codeclimate bot commented Mar 13, 2022

Code Climate has analyzed commit 37d3ba4 and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 6

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

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

View more on Code Climate.

@laurislopata
Copy link
Collaborator Author

Added Jiabao's proposals

@laurislopata laurislopata merged commit 81b80c6 into main Mar 13, 2022
@laurislopata laurislopata deleted the laurislopata/signin branch March 13, 2022 14:31
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.

3 participants