Code Review Suggestions by Jose Chacon.

[ItzSebas10]

Changes Made

MainActivity:

• Removed unnecessary explicit declarations (Saves code typing)
• Removed unused name variable from BuildingName() (SonarQube)
• Changed TextFields Background Color to White (Contrast of 4:5:1 helps with readability - webaim.org)
• Changed variables that never change to val
• Removed unnecessary spaces (Keep code organized and clean)

MainViewModel:

• Removed unused imports (SonarQube)
• Changed variables that never change to val
• Fixed redundancy issues (Kotlin Docs)
• Removed unnecessary spaces

RetrofitClientInstance:

• Made BASE_URL a constant
• Removed keywords to save typing

IBuildingDAO:

• Removed unnecessary spaces

Building:

• Renamed id variable to be more specific ("Choose good names" - Kotlin Docs)
• Removed unused imports and spaces (SonarQube)

BuildingService:

• Removed unnecessary spaces
• Changed the return line code to save typing and omit creating another variable ("Less code is easier to understand and maintain" - Embarcadero)

BuildingUnitTests:

• Removed spaces
• Changed latch variable to val

UI:

• Updated TextFields Background Color (webAIM)

Analysis of the program

Uptown Campus is a really cool idea and has a lot of potential. While I was doing my code review, I found that the team did an excellent job so far, I had a bad time trying to find issues in their code. They did follow the coding conventions extremely well and I noticed few issues during my analysis. The team managed to build the UI, DTOs, DAOs, Retrofit, ViewModel, etc. All of these worked fine during my testing and followed good coding conventions. The team also built unit tests that worked fine, giving a green check for tests passing. At the same time, they made the tests specific so you know what they are actually doing. Organization-wise it was phenomenal, they followed the class layout coding convention and they also did an excellent job at naming each element of the project (variables, functions, etc). Additionally, they used the correct annotations depending on the situation. The team also included comments in their code to explain what certain elements do and furthermore, they did a good job with lambdas and calling functions. The only issues I found were that in some classes, there were unnecessary imports that were never used, variables that were using var instead of val, explicit declarations that could be skipped, some variables that were never used, and some extra spaces that were not needed. In general, I found that their code was excellent and had little to no issues. The team did an excellent job with their code and it also helped me to learn more about coding conventions and make code not only look great but work great as well.

Was the program available in UC Github on time?

Yes, it was. The last edit from the team was on February 23rd.

Is the program documented/commented well enough for you to understand?

Yes, it is. When I was looking at the code of the team, I was able to understand what each function was doing and why they designed certain things the way they did. The team did include comments in certain parts to explain what each thing was doing except for those functions or variables that were self-explanatory. At the same time, due to the organization of the code, it was really easy to locate each element and understand what it was doing. I didn't have any issues reading the code and I was able to understand it correctly.

Does the program compile?

Yes, it does.

The rationale behind your changes

Most of my changes are saving lines of code, removing unnecessary spaces, variables, or imports, and changing the variable types in case it was necessary (for example using val instead of var when possible). I followed different coding conventions such as Kotlin Docs or SonarQube to see what changes could be done. At the same time, used Adobe Color to see if the contrast was 4:5:1 or better.

My three commits on my group's GitHub:

• bracko-3/RentMinder@d1a983a
• bracko-3/RentMinder@72f8a16
• bracko-3/RentMinder@17ef662

The three technical concepts I learned from this code review, were:

Autocomplete TextField: I haven't seen this option before and till now I only knew about the conventional TextField but after looking at the code from the team, I noticed that they added an autocomplete function to the TextField.

Example:

@composable
fun TextFieldWithDropdownUsage(dataIn: List, label: String = "") {
val dropDownOptions = remember { mutableStateOf(listOf()) }
val textFieldValue = remember { mutableStateOf(TextFieldValue()) }
val dropDownExpanded = remember { mutableStateOf(false) }

    fun onDropdownDismissRequest() {
        dropDownExpanded.value = false
    }

    fun onValueChanged(value: TextFieldValue) {
        inBuildingName = value.text
        dropDownExpanded.value = true
        textFieldValue.value = value
        dropDownOptions.value = dataIn.filter {
            it.toString().startsWith(value.text) && it.toString() != value.text
        }.take(3)
    }

    TextFieldWithDropdown(
        modifier = Modifier.fillMaxWidth(),
        value = textFieldValue.value,
        setValue = ::onValueChanged,
        onDismissRequest = ::onDropdownDismissRequest,
        dropDownExpanded = dropDownExpanded.value,
        list = dropDownOptions.value,
        label = label
    )
}

Invoked:

Column {
TextFieldWithDropdownUsage(dataIn = buildings, stringResource(R.string.buildingName))

Firebase: After doing my code review, I managed to learn more about Firebase and how to implement it into an Android Studio project. The team did implement Firebase in their project and while I was looking at the code, I saw how it works.

Example:

private lateinit var firestore : FirebaseFirestore
init {
firestore = FirebaseFirestore.getInstance()
firestore.firestoreSettings = FirebaseFirestoreSettings.Builder().build()
}

fun fetchBuildings() {
    viewModelScope.launch {
        val innerBuildings = buildingService.fetchBuilding()
        buildings.postValue(innerBuildings)
    }
}

fun save(studentComment: StudentComment) {
    val document = if (studentComment.commentId.isEmpty()) {
        firestore.collection("comments").document()
    } else {
        firestore.collection("comments").document(studentComment.commentId)
    }
    studentComment.commentId = document.id
    val handle = document.set(studentComment)
    handle.addOnSuccessListener { Log.d("Firebase", "Document Saved") }
    handle.addOnFailureListener { Log.e("Firebase", "Save failed $it")}
}

Unit Test, ViewModel, and Observer: After looking at the team's project code, I saw how they built the unit test, ViewModel, and Observer and this was really helpful for me. To begin with the Unit Test, I saw how they were able to test the logic of their project and also how specific the unit test was so you can understand what it's doing and what it is about. At the same time, in that same Unit Test, they also used the ViewModel and Observer, and thanks to their code and excellent organization, I was able to understand how these two elements work and what they were doing in the test.

Examples:

UnitTest
@test
fun given a view model with live data when populated with buildings then results show Teachers-Dyer () {
givenViewModelIsInitializedUsingMockData()
whenBuildingServiceFetchBuildingsInvoked()
thenResultsShouldContainTeacherDyer()
}

ViewModel
private fun givenViewModelIsInitializedUsingMockData() {
//Will contain the building objects
val buildings = ArrayList()

    //Hardcoded building objects
    buildings.add(Building(1, "Teachers-Dyer"))
    buildings.add(Building(2, "College of Law"))
    buildings.add(Building(3, "Blegen Library"))
    buildings.add(Building(4, "University Pavilion"))

    coEvery {mockBuildingService.fetchBuilding()} returns buildings

    mainViewModel = MockViewModel(buildingService = mockBuildingService)
}

Observer
private fun thenResultsShouldContainTeacherDyer() {
//capture results
var allBuildings : List? = ArrayList()
val latch = CountDownLatch(1)
val observer = object : Observer<List> {
override fun onChanged(buildingsRecieved: List?) {
allBuildings = buildingsRecieved
latch.countDown()
mainViewModel.buildings.removeObserver(this)
}
}
mainViewModel.buildings.observeForever(observer)
//waits for latch countdown to hit 0
latch.await(10, TimeUnit.SECONDS)

    TestCase.assertNotNull(allBuildings)
    TestCase.assertTrue(allBuildings!!.isNotEmpty())
    var containsTeacher = false
    allBuildings!!.forEach{
        if (it.buildingName.equals("Teachers-Dyer")) {
            containsTeacher = true
        }
    }
    TestCase.assertTrue(containsTeacher)
}

References:

• A. (2022, June 19). The Truth About Software Development Is That Less Code Is Better. Embarcadero RAD Studio, Delphi, & C++Builder Blogs. https://blogs.embarcadero.com/the-truth-about-software-development-is-that-less-code-is-better/
• Basic syntax | Kotlin. (n.d.). Kotlin Help. https://kotlinlang.org/docs/basic-syntax.html
• Coding conventions | Kotlin. (n.d.). Kotlin Help. https://kotlinlang.org/docs/coding-conventions.html
• Clean as you code. (n.d.). https://docs.sonarqube.org/latest/user-guide/clean-as-you-code/
• Lynch, M. (2017, October 12). How to Do Code Reviews Like a Human (Part One). https://mtlynch.io/human-code-reviews-1/
• WebAIM: Web Accessibility In Mind. (n.d.). https://webaim.org/

[ItzSebas10]

You can omit the explicit declaration for MainViewModel (Cleaner code)

Source: Clean as you code. (n.d.). https://docs.sonarqube.org/latest/user-guide/clean-as-you-code/

[ItzSebas10]

The name variable was unnecessary, it was not being used in the BuildingName function. (Clean as you code - SonarQube)

Source: Clean as you code. (n.d.). https://docs.sonarqube.org/latest/user-guide/clean-as-you-code/

[discospiff]

Good.

[ItzSebas10]

1- name variable was never used, I recommend removing it to avoid issues while reading the code. (Clean as you code - SonarQube)
2- You can omit the second explicit declaration of the ArrayList.

Sources:

• Basic syntax | Kotlin. (n.d.). Kotlin Help. https://kotlinlang.org/docs/basic-syntax.html
• Clean as you code. (n.d.). https://docs.sonarqube.org/latest/user-guide/clean-as-you-code/

[discospiff]

Good... though I'd keep the generic identifier on ArrayList... or at least the diamond (<>) operator.

[ItzSebas10]

The variable studentComment never changed so it can be a val instead.

Source:

• Basic syntax | Kotlin. (n.d.). Kotlin Help. https://kotlinlang.org/docs/basic-syntax.html

[discospiff]

Good.

[ItzSebas10]

Unused import (Clean as you code - SonarQube)

Source:

• Clean as you code. (n.d.). https://docs.sonarqube.org/latest/user-guide/clean-as-you-code/

[ItzSebas10]

innerBuildings variable never changed so it can be a val instead.

Source:

• Basic syntax | Kotlin. (n.d.). Kotlin Help. https://kotlinlang.org/docs/basic-syntax.html

[ItzSebas10]

There was some redundancy in the condition which made it a bit more difficult to read. (Avoid redundancy, Kotlin Docs)

Source:

• Coding conventions | Kotlin. (n.d.). Kotlin Help. https://kotlinlang.org/docs/coding-conventions.html

[discospiff]

Is commentId a non-nullable type? In other words, is it declared without a question mark? If so, then yes, I'd remove the null check.

[ItzSebas10]

You can omit the retrofit2 keyword and save some typing.

Source:

• Clean as you code. (n.d.). https://docs.sonarqube.org/latest/user-guide/clean-as-you-code/

[ItzSebas10]

Unused import (Clean as you code - SonarQube).

Source:

• Clean as you code. (n.d.). https://docs.sonarqube.org/latest/user-guide/clean-as-you-code/

[ItzSebas10]

You can save some typing here and just return what the results variable contained. ("Less code is easier to understand and maintain over time" - Embarcadero)

Source:

• A. (2022, June 19). The Truth About Software Development Is That Less Code Is Better. Embarcadero RAD Studio, Delphi, & C++Builder Blogs. https://blogs.embarcadero.com/the-truth-about-software-development-is-that-less-code-is-better/

[iByrd]

This is more typing and honestly more confusing..

[ItzSebas10]

Variable latch never changed so it can be a val instead.

Source:

• Basic syntax | Kotlin. (n.d.). Kotlin Help. https://kotlinlang.org/docs/basic-syntax.html

[ItzSebas10]

To help with readability, it would be good to use a color that has better contrast. White is a good option. (webAim.org)

Source: WebAIM: Web Accessibility In Mind. (n.d.). https://webaim.org/

[ItzSebas10]

To help with the readability of the TextFields, it would be good to use a color that has better contrast. White is a good option. (Minimum contrast of 4:5:1 - webAIM.org)

Source:

• WebAIM: Web Accessibility In Mind. (n.d.). https://webaim.org/

[ItzSebas10]

Renamed id variable to be more specific ("Choose good names" - Kotlin Docs)

Source:

• Coding conventions | Kotlin. (n.d.). Kotlin Help. https://kotlinlang.org/docs/coding-conventions.html

[iByrd]

id was used to match the JSON since I built that too.. I agree with the suggestion, but now this should use SerializeName to work

[discospiff]

Several great suggestions here, which reduce technical debt.

I recommend the group review and consider merging.

[iByrd]

Very nice code review @ItzSebas10 . You did a great job looking into the details of the code and writing your pull request. Your review has been helpful and I am going to implement most changes. Thank you!