Glanzad code review2 by andrewglanzzz · Pull Request #27 · JellyV/FreeWays

andrewglanzzz · 2022-04-02T06:47:06Z

Description of the project:
FreeWays is an application that pulls live traffic / accident data to help you and others stay safe while on the road. It also helps you plan efficient driving routes.

What I learned:
From this code review, I learned quite a bit about cleaning up code. There were quite a few redundancies in the code that I was able to fix and hopefully these suggestions warrant a merge. I also used my previous knowledge of Kotlin naming conventions to fix some various issues that I found within the naming of certain variables. Finally, I learned about function visibility.

… Also removed lateinit from firestore variable as it was unnecessary.

… value.

…ted data hiding on hasCameraPermission() and hasExternalStoragePermission(). I also changed incidentInfo from var -> val since it seems to be immutable.

…rom Retrofit.Builder().

… a value of this type.

…he comment on line 10.

…ightly reduce technical debt.

andrewglanzzz · 2022-04-02T07:00:05Z

Analysis of the program:
I have already reviewed this program, but my main complaint with the program still persists. It is really difficult to figure out how everything is functioning with the lack of comments. I can tell the code is well written and working since the application builds and functions as expected in its current state, I just found it difficult to make changes to the code itself without knowledge of how it works.

Was the program available in UC Github on time?
Yes.

Is the program documented/commented well enough for you to understand?
Not entirely. As stated earlier, comments are practically non-existent except for a few keys areas. This made code readability somewhat troublesome as I had to figure out how everything worked just by reading the code. Comments would've helped a lot.

Does the program compile?
Yes.

Rationale behind your changes.

Redundancy:
This is somewhat self-explanatory. There were a few lines of code I found throughout the program that had redundancy. These were small mistakes that are easy to make, but I figured I would make the changes in order to slightly decrease technical debt.

Renaming:
I renamed some values to abide by Kotlin naming conventions. This does nothing to reduce technical debt, it was just something that needed to be fixed because in a program with multiple variables that are following naming conventions properly, one that is incorrect sticks out like a sore thumb. I also fixed a typo that I fixed on the last pull request, but my change was not merged and more code was written that utilized the val with the typo, which is odd. I fixed both instances of the typo as a result.

Visibility Modifiers:
hasCameraPermission() and hasExternalStoragePermission() needed to be private in my opinion as they are only being used by MainActivity.kt. When dealing with sensitive permissions, ensuring that the functions that work with these are hidden is important.

Links to three specific commits that you made to your own group's GitHub repository before the end of sprint deadline.
brennerdanae/MyLittleLibrary@ed35436
brennerdanae/MyLittleLibrary@58c5724
brennerdanae/MyLittleLibrary@b88a218

Next, list three specific technical concepts that you learned from this code review.
1.) I learned more about Kotlin naming conventions and when to use each type of naming scheme.
2.) I learned about function visibility and how to properly use the private modifier.
3.) I learned about redundancy within Kotlin and the importance of clearing it.

Sources:
https://kotlinlang.org/docs/coding-conventions.html#function-names
https://kotlinlang.org/docs/visibility-modifiers.html#class-members
https://kotlinlang.org/docs/coding-conventions.html#avoid-redundant-constructs

andrewglanzzz · 2022-04-02T07:01:16Z

app/src/main/java/com/ucmobiledevelopment/freeways/AppModule.kt

 @JvmField
 val AppModule = module {
-    viewModel { MainViewModel() }
+    viewModel { MainViewModel(get()) }


I added this as per the request of the comment on line 10. If this is unnecessary or not what you meant, omit it please!

It looks like the MVM constructor requires a parameter... the question is, does Koin know about the parameter type? Is that also defined in Koin?

andrewglanzzz · 2022-04-02T07:02:19Z

app/src/main/java/com/ucmobiledevelopment/freeways/MainActivity.kt

+                        val incidentInfo = Incident().apply {

                            stateId =  0
                            stateName = inStateName


Data within incidentInfo is seems immutable, so I changed it to a val instead of a var.

Good idea. Default to val. Only use var if you know you need to.

andrewglanzzz · 2022-04-02T07:02:45Z

app/src/main/java/com/ucmobiledevelopment/freeways/MainActivity.kt

        var permissionGranted = false
        resultsMap.forEach {
-            if (it.value == true) {
+            if (it.value) {


Removed redundancy. == true was not necessary.

andrewglanzzz · 2022-04-02T07:03:12Z

app/src/main/java/com/ucmobiledevelopment/freeways/MainActivity.kt

-    fun hasCameraPermission() = ContextCompat.checkSelfPermission(this, Manifest.permission.CAMERA)
-    fun hasExternalStoragePermission() = ContextCompat.checkSelfPermission(this, Manifest.permission.WRITE_EXTERNAL_STORAGE)
+    private fun hasCameraPermission() = ContextCompat.checkSelfPermission(this, Manifest.permission.CAMERA)
+    private fun hasExternalStoragePermission() = ContextCompat.checkSelfPermission(this, Manifest.permission.WRITE_EXTERNAL_STORAGE)


Implemented data hiding on these functions as they are only being used in MainActivity,kt

andrewglanzzz · 2022-04-02T07:03:39Z

app/src/main/java/com/ucmobiledevelopment/freeways/MainActivity.kt

            firebaseUser = FirebaseAuth.getInstance().currentUser
            firebaseUser?.let {
-                val user = com.ucmobiledevelopment.freeways.dto.User(it.uid, it.displayName)
+                val user = User(it.uid, it.displayName)


Path was redundant as it was already imported.

Ah, yes. Good one.

andrewglanzzz · 2022-04-02T07:04:18Z

app/src/main/java/com/ucmobiledevelopment/freeways/MainViewModel.kt


 class MainViewModel(var incidentService : IIncidentService = IncidentService()) : ViewModel(){
-    internal val NEW_INCIDENT = "New Incident"
+    private val NEW_INCIDENT = "New Incident"


Changed internal -> private as it is only being used here and is deeply immutable. If you plan on using NEW_INCIDENT somewhere else, ignore this.

andrewglanzzz · 2022-04-02T07:05:24Z

app/src/main/java/com/ucmobiledevelopment/freeways/MainViewModel.kt

-    private lateinit var firestore: FirebaseFirestore
+    private var firestore: FirebaseFirestore = FirebaseFirestore.getInstance()

    init{
-        firestore = FirebaseFirestore.getInstance()


lateinit seemed unnecessary, so I removed the lateinit modifier and got the instance as the var was declared.

Either way works. I'd probably go with what the reviewer recommends here.

andrewglanzzz · 2022-04-02T07:05:41Z

app/src/main/java/com/ucmobiledevelopment/freeways/ReportIncidentActivity.kt

        super.onCreate(savedInstanceState)
        setContent {
-            FreeWaysTheme() {
+            FreeWaysTheme {


Small change, but these parentheses are not necessary here.

andrewglanzzz · 2022-04-02T07:06:13Z

app/src/main/java/com/ucmobiledevelopment/freeways/ReportIncidentActivity.kt

                val context = LocalContext.current
                ExtendedFloatingActionButton (
-                    text = {  Text(text = "Back") },
+                    text = { Text(text = "Back") },


Incredibly small change, just removed an extra space because I happened to notice it.

andrewglanzzz · 2022-04-02T07:06:31Z

app/src/main/java/com/ucmobiledevelopment/freeways/dto/IncidentDTO.kt

    val FATALS: String? = "",
    val LATITUDE: String? = "",
-    val LONGITUD: String? = "",
+    val LONGITUDE: String? = "",


Fixed typo on LONGITUDE.

andrewglanzzz · 2022-04-02T07:06:48Z

app/src/main/java/com/ucmobiledevelopment/freeways/service/IncidentService.kt

                        newIncident.stateName = it.STATENAME ?: ""
                        newIncident.latitude = it.LATITUDE ?: ""
-                        newIncident.longitude = it.LONGITUD ?: ""
+                        newIncident.longitude = it.LONGITUDE ?: ""


Fixed typo on LONGITUDE reference.

andrewglanzzz · 2022-04-02T07:07:26Z

app/src/main/java/com/ucmobiledevelopment/freeways/RetrofitClientInstance.kt

            if (retrofit == null) {
                // create it
-                retrofit = retrofit2.Retrofit.Builder()
+                retrofit = Retrofit.Builder()


I understand this is boilerplate, but specifying retrofit2 is not necessary as it has already been imported.

andrewglanzzz · 2022-04-02T07:07:58Z

app/src/main/java/com/ucmobiledevelopment/freeways/service/IncidentService.kt

                if(result.Results[0].isNotEmpty()){
                    result.Results[0].forEach{
-                        val newIncident: Incident = Incident()
+                        val newIncident = Incident()


Redundancy on newIncident. Shortened it up.

andrewglanzzz · 2022-04-02T07:08:17Z

app/src/test/java/com/ucmobiledevelopment/freeways/IncidentTests.kt

       var hasCaseId400434 = false
       allIncidents!!.forEach {
-             if(it.caseId.equals("400434")){
+             if(it.caseId == "400434"){


Shifted this over to a binary operator to reduce technical debt.

andrewglanzzz · 2022-04-02T07:09:58Z

app/src/main/java/com/ucmobiledevelopment/freeways/ui/theme/Theme.kt

-private val DarkColorPalette = darkColors(
+private val darkColorPalette = darkColors(
    primary = Purple200,
    primaryVariant = Purple700,
    secondary = Teal200
 )

-private val LightColorPalette = lightColors(
+private val lightColorPalette = lightColors(


Changed these to fit Kotlin naming conventions. Camel Case seems appropriate considering the data that these values hold.

andrewglanzzz · 2022-04-02T07:10:20Z

app/src/main/java/com/ucmobiledevelopment/freeways/ui/theme/Theme.kt

-        DarkColorPalette
+        darkColorPalette
    } else {
-        LightColorPalette
+        lightColorPalette


Changed the names when called to fix errors caused by renaming.

discospiff · 2022-04-03T15:09:40Z

Several good suggestions here that reduce technical debt. I recommend the group merge this branch.

andrewglanzzz added 8 commits April 2, 2022 01:49

Changed visibility on NEW_INCIDENT to private as opposed to internal.…

65dc59b

… Also removed lateinit from firestore variable as it was unnecessary.

Removed redundancy on newIncident value and fixed a typo on LONGITUDE…

989db86

… value.

Simplified a boolean expression and removed redundancy. Also implemen…

3a11f66

…ted data hiding on hasCameraPermission() and hasExternalStoragePermission(). I also changed incidentInfo from var -> val since it seems to be immutable.

Since retrofit2 has already been imported, I removed the redundancy f…

9244112

…rom Retrofit.Builder().

Adjusted val naming to fit Kotlin naming conventions (camel case) for…

fd61b1e

… a value of this type.

Added get() as the parameter in MainViewModel as per the request of t…

5948eed

…he comment on line 10.

Removed unnecessary parentheses from FreeWaysTheme.

e3acca5

Changed caseId.equals to utilize a binary operator. Should ever so sl…

5a26864

…ightly reduce technical debt.

andrewglanzzz commented Apr 2, 2022

View reviewed changes

Conversation

andrewglanzzz commented Apr 2, 2022

Uh oh!

andrewglanzzz commented Apr 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

discospiff commented Apr 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andrewglanzzz commented Apr 2, 2022 •

edited

Loading