Add postID:Int to Post, format files to follow kotlin style guide, ad…#8
Add postID:Int to Post, format files to follow kotlin style guide, ad…#8ccbrowndev wants to merge 1 commit intomainfrom
Conversation
…d internet permission to manifest
| <manifest xmlns:android="http://schemas.android.com/apk/res/android" | ||
| package="com.example.uniconnect"> | ||
|
|
||
| <uses-permission android:name="android.permission.INTERNET"/> |
There was a problem hiding this comment.
Later when you add an interface, the app will want this permission to hit the API
| Surface( | ||
| modifier = Modifier.fillMaxSize(), | ||
| color = MaterialTheme.colors.background | ||
| ) { |
There was a problem hiding this comment.
automatic via kotlin style guide per https://kotlinlang.org/docs/coding-conventions.html#apply-the-style-guide
| private val BASE_URL = "http://universities.hipolabs.com/" | ||
|
|
||
| val retrofitInstance : Retrofit? | ||
| val retrofitInstance: Retrofit? |
There was a problem hiding this comment.
automatic via kotlin style guide per https://kotlinlang.org/docs/coding-conventions.html#apply-the-style-guide
| */ | ||
| @GET("/search") | ||
| fun getAllUniversities() : Call<ArrayList<University>> | ||
| fun getAllUniversities(): Call<ArrayList<University>> |
There was a problem hiding this comment.
automatic via kotlin style guide per https://kotlinlang.org/docs/coding-conventions.html#apply-the-style-guide
| package com.example.uniconnect.dto | ||
|
|
||
| data class Post(var title: String, var description: String) { | ||
| data class Post(var postID: Int, var title: String, var description: String) { |
There was a problem hiding this comment.
This will enable you to have posts that are titled the same but made at different times or places
There was a problem hiding this comment.
This looks like an incomplete feature.
Are you using these anywhere? I don't see them used.
If not in use, remove them. Unused code is clutter, and bad documentation, which increases technical debt.
Only complete features, which meet the team's Definition of Done, should be merged to Main/Master.
| data class University( | ||
| @SerializedName("name") var name: String, | ||
| @SerializedName("country") var country: String, | ||
| @SerializedName("alpha_two_code") var code: String | ||
| ) { |
There was a problem hiding this comment.
automatic via kotlin style guide per https://kotlinlang.org/docs/coding-conventions.html#apply-the-style-guide
| val retrofit = | ||
| RetrofitClientInstance.retrofitInstance?.create(IUniversityDAO::class.java) | ||
| val universities = async { retrofit?.getAllUniversities() } |
There was a problem hiding this comment.
automatic via kotlin style guide per https://kotlinlang.org/docs/coding-conventions.html#apply-the-style-guide
| var rule: TestRule = InstantTaskExecutorRule() | ||
| lateinit var universityService: IUniversityService | ||
| var allUniversities : List<University>? = ArrayList<University>() | ||
| var allUniversities: List<University>? = ArrayList<University>() |
There was a problem hiding this comment.
automatic via kotlin style guide per https://kotlinlang.org/docs/coding-conventions.html#apply-the-style-guide
| launch(Dispatchers.Main) { | ||
| givenUniversityServiceIsInitialized() | ||
| whenServiceDataAreReadAndParsed() | ||
| thenTheUniversityCollectionSizeShouldBeGreaterThanZero() | ||
| } |
There was a problem hiding this comment.
automatic via kotlin style guide per https://kotlinlang.org/docs/coding-conventions.html#apply-the-style-guide
| fun `Given a post DTO when title is CCM Concert and description is In Corbet Auditorium then output is CCM Concert - In Corbet Auditorium`(){ | ||
| val post = Post("CCM Concert", "In Corbet Auditorium") | ||
| fun `Given a post DTO when title is CCM Concert and description is In Corbet Auditorium then output is CCM Concert - In Corbet Auditorium`() { | ||
| val post = Post(1, "CCM Concert", "In Corbet Auditorium") |
There was a problem hiding this comment.
adding a mock ID so the tests continue to work as before
| var containsMarywoodUniversity = false | ||
| allUniversities!!.forEach { | ||
| if (it.name.equals("Marywood University") && it.code.equals("US")) { | ||
| containsMarywoodUniversity = true | ||
| } | ||
| } | ||
| assertTrue(containsMarywoodUniversity) |
There was a problem hiding this comment.
This checks that actual results were pulled because it is possible to get an array of empty string that will still count as not empty
|
Things I learned: I learned about the kotlin style guide and how you can set android studio to automatically suggest formatting fixes beyond the usual. I learned that tests will pass without internet permission even though the app itself may fail on a device without this permission I learned that according to the kotlin style guide, variables should be placed on separate lines if they would make the single line very long as seen in https://github.com/pranavm7/UniConnect/blob/4eb8a21f189936f6f8973356877939b1b20577fd/UniConnectApp/app/src/main/java/com/example/uniconnect/dto/University.kt |
|
Commits I made in my own group project: |
| assertNotNull(allUniversities) | ||
| assertTrue(allUniversities!!.isNotEmpty()) | ||
| var containsMarywoodUniversity = false | ||
| allUniversities!!.forEach { |
There was a problem hiding this comment.
Use ?.let{} instead of force unwrap (!!) to handle nullness.
|
Adding more unit tests is a good idea; see my note on nullness. I would not add incomplete functionality, until it is complete. The IDE can format automatically with Control Shift Alt L. So, I wouldn't merge this branch as is, but consider the suggestion to enhance test coverage. |
2 participants
Changes:
Added to a test in the file to verify that an actual result came back instead of just being non-empty
Added postID variable to Post
Formatted several files to fit the kotlin style guide better
Added internet permission to the android manifest
Analysis:
The program looks like it will have various universities and users will be able to view posts by these universities. They have dto for posts and universities, and service to pull university data from the web
It was available on github on time and compiled and ran. It was also documented well enough for me to understand
Rationale for my changes:
I learned during my individual assignment that it is possible to get unexpected results from an API, specifically that I would receive an array full of empty strings, which is not an empty array. By checking an actual result it is much more likely to verify that the API was hit properly.
Universities may post often, titles may get re-used. Having a number to identify them makes it far less likely that there will be some conflict
The internet permission I added was just a formality that will be required for the app in the emulator to work properly
The kotlin style things were done automatically via https://kotlinlang.org/docs/coding-conventions.html#apply-the-style-guide and using alt-shift-enter within android studio