Migrate from ktlint to ktfmt - #2832
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces project-wide code formatting and style updates across multiple Firebase quickstart modules, replacing ktlint with Spotless for Kotlin code style enforcement. The code review identified several critical and medium-severity issues, primarily concerning the unsafe use of the force-unwrap operator (!!) on nullable objects (such as databaseError, auth.currentUser, dataSnapshot.key, and task.result?.user) which could lead to NullPointerException crashes. Additionally, a potential division-by-zero bug was flagged in RatingUtil.kt when calculating the average rating of an empty list.
| currentData: DataSnapshot?, | ||
| ) { | ||
| // Transaction completed | ||
| Log.d(TAG, "postTransaction:onComplete:" + databaseError!!) |
There was a problem hiding this comment.
CRITICAL BUG: databaseError is null when the transaction completes successfully. Force-unwrapping it with !! will guarantee a NullPointerException and crash the app on every successful transaction. Remove the force-unwrap operator.
| Log.d(TAG, "postTransaction:onComplete:" + databaseError!!) | |
| Log.d(TAG, "postTransaction:onComplete:$databaseError") |
| showProgressBar() | ||
|
|
||
| auth.currentUser!!.linkWithCredential(credential).addOnCompleteListener(requireActivity()) { |
There was a problem hiding this comment.
Using the force-unwrap operator !! on auth.currentUser can lead to a NullPointerException if the user session expires or becomes null. It is safer to use optional binding or a guard clause to handle the nullable FirebaseUser? safely.
| showProgressBar() | |
| auth.currentUser!!.linkWithCredential(credential).addOnCompleteListener(requireActivity()) { | |
| val currentUser = auth.currentUser ?: return | |
| showProgressBar() | |
| currentUser.linkWithCredential(credential).addOnCompleteListener(requireActivity()) { |
| binding.verifyEmailButton.isEnabled = false | ||
|
|
||
| // Send verification email | ||
| val user = auth.currentUser!! |
There was a problem hiding this comment.
Using the force-unwrap operator !! on auth.currentUser is unsafe and can cause a crash if the user is not signed in. Use a safe guard clause instead.
| val user = auth.currentUser!! | |
| val user = auth.currentUser ?: return |
| val credential = PhoneAuthProvider.getCredential(lastCodeVerificationId!!, smsCode) | ||
| enrollWithPhoneAuthCredential(credential) | ||
| } | ||
| Firebase.auth.currentUser!!.multiFactor.session.addOnCompleteListener { task -> |
There was a problem hiding this comment.
Using the force-unwrap operator !! on Firebase.auth.currentUser is unsafe. Use a safe guard clause to handle the nullable FirebaseUser? safely.
val currentUser = Firebase.auth.currentUser ?: return
currentUser.multiFactor.session.addOnCompleteListener { task ->| private fun sendEmailVerification() { // Disable button | ||
| binding.verifyEmailButton.isEnabled = false | ||
| // Send verification email | ||
| val user = auth.currentUser!! |
There was a problem hiding this comment.
Using the force-unwrap operator !! on auth.currentUser is unsafe. Use a safe guard clause instead.
| val user = auth.currentUser!! | |
| val user = auth.currentUser ?: return |
| } | ||
| } | ||
| if (task.isSuccessful) { | ||
| onAuthSuccess(task.result?.user!!) |
There was a problem hiding this comment.
Using task.result?.user!! is redundant and unsafe. Since task.result is nullable, safe-navigating it and then force-unwrapping the user can still crash if the user is null. Use optional let binding instead.
| onAuthSuccess(task.result?.user!!) | |
| task.result?.user?.let { onAuthSuccess(it) } |
| Log.d(TAG, "onChildAdded:" + dataSnapshot.key!!) | ||
|
|
||
| private fun postComment() { | ||
| val uid = uid | ||
| Firebase.database.reference.child("users").child(uid) | ||
| .addListenerForSingleValueEvent(object : ValueEventListener { | ||
| override fun onDataChange(dataSnapshot: DataSnapshot) { | ||
| // Get user information | ||
| val user = dataSnapshot.getValue<User>() ?: return | ||
| // A new comment has been added, add it to the displayed list | ||
| val comment = dataSnapshot.getValue<Comment>() | ||
|
|
||
| val authorName = user.username | ||
| // Update RecyclerView | ||
| commentIds.add(dataSnapshot.key!!) | ||
| comments.add(comment!!) | ||
| notifyItemInserted(comments.size - 1) |
There was a problem hiding this comment.
Force-unwrapping dataSnapshot.key!! and comment!! can cause a crash if the key or the comment value is null (e.g., due to malformed or deleted database entries). It is safer to use guard clauses to return early if either is null.
| Log.d(TAG, "onChildAdded:" + dataSnapshot.key!!) | |
| private fun postComment() { | |
| val uid = uid | |
| Firebase.database.reference.child("users").child(uid) | |
| .addListenerForSingleValueEvent(object : ValueEventListener { | |
| override fun onDataChange(dataSnapshot: DataSnapshot) { | |
| // Get user information | |
| val user = dataSnapshot.getValue<User>() ?: return | |
| // A new comment has been added, add it to the displayed list | |
| val comment = dataSnapshot.getValue<Comment>() | |
| val authorName = user.username | |
| // Update RecyclerView | |
| commentIds.add(dataSnapshot.key!!) | |
| comments.add(comment!!) | |
| notifyItemInserted(comments.size - 1) | |
| val key = dataSnapshot.key ?: return | |
| val comment = dataSnapshot.getValue<Comment>() ?: return | |
| Log.d(TAG, "onChildAdded:$key") | |
| // Update RecyclerView | |
| commentIds.add(key) | |
| comments.add(comment) | |
| notifyItemInserted(comments.size - 1) |
| if (commentIndex > -1) { | ||
| // Remove data from the list | ||
| commentIds.removeAt(commentIndex) | ||
| comments.removeAt(commentIndex) | ||
|
|
||
| // Update the RecyclerView | ||
| notifyItemRemoved(commentIndex) | ||
| } else { | ||
| Log.w(TAG, "onChildRemoved:unknown_child:" + commentKey!!) | ||
| } | ||
| databaseReference.addChildEventListener(childEventListener) | ||
| } | ||
|
|
||
| // Store reference to listener so it can be removed on app stop | ||
| this.childEventListener = childEventListener | ||
| } | ||
| override fun onChildMoved(dataSnapshot: DataSnapshot, previousChildName: String?) { | ||
| Log.d(TAG, "onChildMoved:" + dataSnapshot.key!!) | ||
|
|
||
| override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): CommentViewHolder { | ||
| val inflater = LayoutInflater.from(context) | ||
| val view = inflater.inflate(R.layout.item_comment, parent, false) | ||
| return CommentViewHolder(view) | ||
| } | ||
| // A comment has changed position, use the key to determine if we are |
There was a problem hiding this comment.
Force-unwrapping dataSnapshot.key!! and commentKey!! can cause a crash if the key is null. It is safer to use guard clauses to return early if the key is null.
| if (commentIndex > -1) { | |
| // Remove data from the list | |
| commentIds.removeAt(commentIndex) | |
| comments.removeAt(commentIndex) | |
| // Update the RecyclerView | |
| notifyItemRemoved(commentIndex) | |
| } else { | |
| Log.w(TAG, "onChildRemoved:unknown_child:" + commentKey!!) | |
| } | |
| databaseReference.addChildEventListener(childEventListener) | |
| } | |
| // Store reference to listener so it can be removed on app stop | |
| this.childEventListener = childEventListener | |
| } | |
| override fun onChildMoved(dataSnapshot: DataSnapshot, previousChildName: String?) { | |
| Log.d(TAG, "onChildMoved:" + dataSnapshot.key!!) | |
| override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): CommentViewHolder { | |
| val inflater = LayoutInflater.from(context) | |
| val view = inflater.inflate(R.layout.item_comment, parent, false) | |
| return CommentViewHolder(view) | |
| } | |
| // A comment has changed position, use the key to determine if we are | |
| val commentKey = dataSnapshot.key ?: return | |
| Log.d(TAG, "onChildRemoved:$commentKey") | |
| val commentIndex = commentIds.indexOf(commentKey) | |
| if (commentIndex > -1) { | |
| // Remove data from the list | |
| commentIds.removeAt(commentIndex) | |
| comments.removeAt(commentIndex) | |
| // Update the RecyclerView | |
| notifyItemRemoved(commentIndex) | |
| } else { | |
| Log.w(TAG, "onChildRemoved:unknown_child:$commentKey") | |
| } |
| // ... | ||
| } | ||
|
|
||
| override fun onCancelled(databaseError: DatabaseError) { | ||
| Log.w(TAG, "postComments:onCancelled", databaseError.toException()) | ||
| Toast.makeText(context, "Failed to load comments.", Toast.LENGTH_SHORT).show() |
There was a problem hiding this comment.
Force-unwrapping dataSnapshot.key!! can cause a crash if the key is null. It is safer to use guard clauses to return early if the key is null.
| // ... | |
| } | |
| override fun onCancelled(databaseError: DatabaseError) { | |
| Log.w(TAG, "postComments:onCancelled", databaseError.toException()) | |
| Toast.makeText(context, "Failed to load comments.", Toast.LENGTH_SHORT).show() | |
| val commentKey = dataSnapshot.key ?: return | |
| Log.d(TAG, "onChildMoved:$commentKey") | |
| // A comment has changed position, use the key to determine if we are | |
| // displaying this comment and if so move it. | |
| val movedComment = dataSnapshot.getValue<Comment>() |
| fun getAverageRating(ratings: List<Rating>): Double { | ||
| var sum = 0.0 | ||
|
|
||
| return sum / ratings.size | ||
| for (rating in ratings) { | ||
| sum += rating.rating | ||
| } | ||
|
|
||
| return sum / ratings.size | ||
| } |
There was a problem hiding this comment.
If the ratings list is empty, ratings.size is 0, which will result in a division by zero. In Kotlin/Java, Double division by zero returns NaN, which can cause unexpected behavior or failures when writing to Firestore. Add a guard clause to return 0.0 if the list is empty.
fun getAverageRating(ratings: List<Rating>): Double {
if (ratings.isEmpty()) {
return 0.0
}
var sum = 0.0
for (rating in ratings) {
sum += rating.rating
}
return sum / ratings.size
}…rpf/migrate-to-ktfmt # Conflicts: # admob/app/build.gradle.kts # analytics/app/build.gradle.kts # appdistribution/app/build.gradle.kts # auth/app/build.gradle.kts # config/app/build.gradle.kts # crash/app/build.gradle.kts # database/app/build.gradle.kts # firestore/app/build.gradle.kts # functions/app/build.gradle.kts # inappmessaging/app/build.gradle.kts # messaging/app/build.gradle.kts # perf/app/build.gradle.kts # storage/app/build.gradle.kts
1 participant
No description provided.