Skip to content

Feature/impl db#105

Open
JohnLeeroy wants to merge 10 commits intocforlando:developfrom
JohnLeeroy:feature/impl_db
Open

Feature/impl db#105
JohnLeeroy wants to merge 10 commits intocforlando:developfrom
JohnLeeroy:feature/impl_db

Conversation

@JohnLeeroy
Copy link
Copy Markdown
Contributor

Room Database implemented. We are using two tables, Animals and Images. Through the AnimalAndImageDao, you have all the properties to map it to Animal.

Changes:
Converted PetfinderProvider to Kotlin
Updated support library to 27.1
Updated SDK to 26 while making roomdb compatible with project.

super.onCreate(savedInstanceState)
setContentView(R.layout.activity_about)
val toolbar = findViewById(R.id.toolbar) as Toolbar
val toolbar = findViewById<Toolbar>(R.id.toolbar)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's generally a lot cleaner to use generic inference rather than explicitly defining the generic and relying on type inference such as

val toolbar: Toolbar = findViewById(R.id.toolbar)

In at least one use case you ended up specifying it both ways.

bind()
}

fun bind() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider using getters instead of a large bind method.

private val logo: ImageView
   get() = findViewById(R.id.cfo_logo)

.autoConnect()
}

private fun toAnimalList(response: PetfinderPetRecordResponse): List<Animal> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A more functional approach would be this.

private fun toAnimalList(response: PetfinderPetRecordResponse): List<Animal>
 = response.petfinder
   .pets
   .petList
   .map { animal ->
      Animal.AnimalBuilder()
         ...
         .createAnimal()
   }

return outputAnimals
}

private fun photosToStringUrls(photos: PetfinderAnimal.Media.Photos?): List<String> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider the previously mentioned functional approach

return urlList
}

private fun getBreed(breed: PetfinderAnimal.Breeds.Breed?): String {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be consolidated to a single line

return bread?.let{ breed.content } ?: ""

This is the logical equivalent of

if (breed != null) {
   return breed.content;
}

return "";


class AnimalMapper @Inject constructor(){

fun map(from: Animal): AnimalEntity {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would strongly suggest avoiding the function name of map in kotlin as it will be confusing to a reader. A more specific name would remove any confusion. mapToAnimalEntity

import javax.inject.Inject


class AnimalMapper @Inject constructor(){
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be a utility class which means it should not be a class in kotlin. Instead of placeholder classes purely to provide extension functionality just write extension functions and get rid of the container class.

fun Animal.mapToAnimalEntity(): AnimalEntity
   ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants