Let's Review: Pokedex

Introduction

Last week, a project caught my eye on the /r/androiddev subreddit. It was very, very good looking, so it got a lot of attention. There were a few comments about the implementation itself in the discussions below, but not a lot. So I’ve decided to take a look at it, and write up my thoughts on the code.

A couple things to clarify here, before we get into it:

• The project is a work-in-progress side project by the author, who’s making it in their spare time, as far as I understand. This means that it’s both incomplete at this point, and it’s not meant to be a huge production application. So let’s appriciate this for the great effort that it is, going into learning the new and shiny parts of Android development!
• This review is meant to be a learning resource, so that both the original author and anyone who reads this might learn from it. I believe that there’s a good chance that many people might run into the same kinds of mistakes, and I hope this might help them.

With that, let’s get started! You’ll find commit hashes (abc1234) in the article, if you want to look at how each change can be applied to the project. You can also check out the fork’s repository. There is also an open pull request with these changes!

So is this the start of a new series? If it’s well received, perhaps it is. Or I may never do this again - we’ll see!

Project overview

The project structure overall seems well thought out, and makes sense. Navigating it was very easy. The UI parts of the application are grouped nicely screen by screen, Fragments and ViewModels.

The only odd bit here was that all the RecyclerView.Adapter implementations lived in their own adapter package. I would at least move this adapter package inside the ui package, and if they’re not reused between screens, also inside the respective sub-packages.

03a41df

---

While going through the files, I’ve noticed that some of the files are not auto-formatted, and some have unused imports in them. This can be fixed easily by just running Reformat Code with Optimize imports selected over the entire codebase.

Of course, this doesn’t really matter in the grand scheme of things, but things like this are worth cleaning up. This being a WIP project explains why this hasn’t been done yet, but it’s probably better to do this continuously during development.

964d5f7

Android, Jetpack, and lifecycles

Let’s get into some of Android’s specifics, as well as how the new Jetpack components interact with the framework.

---

Let’s take a look at ViewModel initialization, which happens in the Fragments like this:

class PokedexFragment : Fragment() {
    private lateinit var pokedexViewModel: PokedexViewModel

    override fun onCreateView(...) : View? {
        pokedexViewModel = ViewModelProviders.of(this)
                                 .get(PokedexViewModel::class.java)
        ...
    }
}

Two caveats here:

• The Fragment has lifecycle methods that run before onCreateView does, including onCreate. If the ViewModel is initialized only in onCreateView, then in these earlier methods, it won’t be available for use yet, and accessing it would result in a crash.
• The onCreateView method can run several times during a Fragment’s lifecycle, as its view is destroyed and recreated, while the Fragment instance remains the same. When this happens, unnecessary calls to ViewModelProviders will be made here (although this won’t cause a bug, as the previous ViewModel instance will be returned again).

To fix these, we can move ViewModel initialization to the onCreate method (or even onAttach, if you prefer):

override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    pokedexViewModel = ViewModelProviders.of(this)
                             .get(PokedexViewModel::class.java)
}

b2c294c

---

While we’re at the onCreateView method, LiveData observations on the ViewModel are also being set up in here:

class PokedexFragment : Fragment() {
    override fun onCreateView(...) : View? {
        ...
        pokedexViewModel.getListPokemon().observe(this, Observer {
            ...
        })
        ...
    }
}

This will attach a new Observer instance to the LiveData each time the Fragment creates a new view for itself. Since this, the Fragment, is being used as the LifecycleOwner for the observation, these observers will only be cleared up when the Fragment is entirely destroyed.

To keep these observations from stacking like this, viewLifecycleOwner should be used instead:

class PokedexFragment : Fragment() {
    override fun onCreateView(...) : View? {
        ...
        pokedexViewModel.getListPokemon().observe(viewLifecycleOwner, Observer {
            ...
        })
        ...
    }
}

95ebf91

This lifecycle ends when the Fragment’s view is destroyed, cleaning up the Observer appropriately. When a new view is created, the method will run again, attaching a new Observer.

---

In the onCreateView method, initializing the Fragment’s view is slightly tricky, because it’s not set yet (getView() won’t return it, for example), as we’ve yet to pass it back to the framework. That only happens at the end of the method.

This results in code like this, where the inflated View is stored in a local variable, then initialization happens, and finally it’s returned.

override fun onCreateView(
    inflater: LayoutInflater,
    container: ViewGroup?,
    savedInstanceState: Bundle?
): View? {
    val root = inflater.inflate(R.layout.fragment_home, container, false)
    val recyclerViewMenu = root.recyclerViewMenu

    recyclerViewMenu.layoutManager = GridLayoutManager(context, 2)

    return root
}

Whether you’re using findViewById or Kotlin synthetics, this means that you have to save the View instances that you’ve looked up in the inflated View to local variables to work on them - just like in the code snippet above.

There is a better way to do this! The onViewCreated lifecycle method runs right after onCreateView returns and the View it produces is set in the Fragment. Having only the inflation happen in onCreateView and moving the rest of your initialization code to onViewCreated separates these two tasks, and makes for simpler code.

override fun onCreateView(
    inflater: LayoutInflater,
    container: ViewGroup?,
    savedInstanceState: Bundle?
): View? {
    return inflater.inflate(R.layout.fragment_home, container, false)
}

Kotlinx synthetics are also easier to use if you use them in onViewCreated instead of in onCreateView:

override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
    super.onViewCreated(view, savedInstanceState)
    recyclerViewMenu.layoutManager = GridLayoutManager(context, 2)
}

Here you don’t have to keep a reference to the Fragment’s root view manually, and you don’t have to save the resolved View instances to local variables either.

7385d6a

---

The PokedexViewModel class contains the following line:

listPokemon.value = pokemonDAO.all().value

On the right hand side here, a LiveData is being fetched from Room, and then its value is being read immediately. This will more than likely always return null, as the value will be placed in the LiveData asynchronously, which is why LiveData should always be observed. If you have to pull a value from a LiveData like this, something has almost certainly gone wrong.

This doesn’t result in a bug in the application, because listPokemon is actually unused at this point.

3916b47

---

A small thing to wrap up this section, let’s take a look at the convertColor method of the PokemonColorUtil class:

fun convertColor(color: Int): Int {
    return Color.parseColor("#" + 
             Integer.toHexString(ContextCompat.getColor(context, color)))
}

The ContextCompat.getColor(context, color)) call here already returns a ColorInt, an AARRGGBB representation of a color in a single Int. Then, the method transforms this to a hex String just to parse it back into a ColorInt again. As far as I can tell, this is redudant, so we can simplify the code:

@ColorInt
fun convertColor(@ColorRes color: Int): Int {
    return ContextCompat.getColor(context, color)
}

I’ve also added the @ColorRes and @ColorInt annotations to the method signature, to clarify what the input and output Int values contain.

e9625b1

---

Getting back to adapters for a moment, the app has a ViewPagerAdapter that’s filled with Fragment instances dynamically:

val adapter = ViewPagerAdapter(fragmentManager!!)
adapter.addFragment(AboutFragment.newInstance(pokemon?.id), getString(R.string.dashboard_tab_1))
adapter.addFragment(StatsFragment.newInstance(pokemon?.id), getString(R.string.dashboard_tab_2))
adapter.addFragment(EvolutionFragment(), getString(R.string.dashboard_tab_3))
adapter.addFragment(MovesFragment(), getString(R.string.dashboard_tab_4))

And it keeps track of these Fragments in a list internally:

class ViewPagerAdapter(...) : FragmentStatePagerAdapter(...) {
    private val mFragmentList = ArrayList<Fragment>()
    private val mFragmentTitleList = ArrayList<String>()
    ...
}

For the explanation of why this is a bad idea, I’ll point you to this article’s item #3, as its author brought up this issue in the reddit comments anyway.

Hungarian notation should also be abolished from here! Say mNo to this.

Let’s see how we can fix this, by moving Fragment creation into the getItem call of the adapter (as the adapter doesn’t need to be completely dynamic anyway):

class ViewPagerAdapter(
    supportFragmentManager: FragmentManager,
    private val context: Context,
    private val pokemonId: String
) : FragmentStatePagerAdapter(supportFragmentManager, ...) {

    override fun getCount(): Int = 4

    override fun getItem(position: Int): Fragment {
        return when (position) {
            0 -> AboutFragment.newInstance(pokemonId)
            1 -> StatsFragment.newInstance(pokemonId)
            2 -> EvolutionFragment()
            3 -> MovesFragment()
            else -> throw IllegalArgumentException("Unhandled position")
        }
    }

    override fun getPageTitle(position: Int): CharSequence? {
        return when (position) {
            0 -> context.getString(R.string.dashboard_tab_1)
            1 -> context.getString(R.string.dashboard_tab_2)
            2 -> context.getString(R.string.dashboard_tab_3)
            3 -> context.getString(R.string.dashboard_tab_4)
            else -> throw IllegalArgumentException("Unhandled position")
        }
    }
}

Or, to make the code a bit more robust, you might group the titles and constructor calls into page objects, with something like this:

class ViewPagerAdapter(
    supportFragmentManager: FragmentManager,
    context: Context,
    private val pokemonId: String
) : FragmentStatePagerAdapter(supportFragmentManager, ...) {

    data class Page(val title: String, val ctor: () -> Fragment)

    private val pages = listOf(
        Page(
            context.getString(R.string.dashboard_tab_1),
            { AboutFragment.newInstance(pokemonId) }
        ),
        Page(
            context.getString(R.string.dashboard_tab_2),
            { StatsFragment.newInstance(pokemonId) }
        ),
        Page(
            context.getString(R.string.dashboard_tab_3),
            { EvolutionFragment() }
        ),
        Page(
            context.getString(R.string.dashboard_tab_4),
            { MovesFragment() }
        )
    )

    override fun getItem(position: Int): Fragment {
        return pages[position].ctor()
    }

    override fun getCount(): Int {
        return pages.size
    }

    override fun getPageTitle(position: Int): CharSequence? {
        return pages[position].title
    }
}

This way you don’t have to update three different functions when modifying the pages, just add a new list element.

2cf3ba9

Kotlin

The project doesn’t have any kind of dependency injection in it. This is reasonable for a small toy project, but the lack of DI still results in code that could be cleaned up a bit.

First, let’s take a look at the Application implementation:

class App : Application() {
    companion object {
        var context: Context? = null
        var database: AppDatabase? = null
    }

    override fun onCreate() {
        super.onCreate()
        context = this
        database = Room.databaseBuilder(...).build()
    }
}

This App class provides both a Context and an AppDatabase instance globally, to the entire application. Whenever one of these things is needed, it’s just pulled from the companion object, like this:

App.context!!.resources.getString(R.string.generation_item_1)
App.database!!.pokemonDAO()

Since they don’t have an initial value when the companion object is created, but before App#onCreate executed, these two properties had to be declared as nullable, and therefore they have to be asserted as non-null on every use site.

lateinit is Kotlin’s solution for this exact scenario, when a property will never be null by the time that it’s used, but can’t be initialized at constructor time:

companion object {
    lateinit var context: Context
    lateinit var database: AppDatabase
}

This is just as unsafe as what we had before - if we forget to initialize these properties before using them, we’ll crash when trying to access their values. However, their use sites don’t have to handle nullability any more:

App.context.resources.getString(R.string.generation_item_1)
App.database.pokemonDAO()

5c3dca5

---

Another place in the code where DI is substituted is the APIService class, which acts as a factory for the Retrofit-generated API implementation:

class APIService {
    private val retrofit = Retrofit.Builder()
        .baseUrl("...")
        .addConverterFactory(GsonConverterFactory.create())
        .build()

    fun pokemonService(): PokemonService {
        return retrofit.create(PokemonService::class.java)
    }
}

This class is instantiated every time that an API call has to be made:

val call = APIService().pokemonService().get()

This creates new Retrofit instances each time, as well as a new PokemonService implementation.

This APIService might as well be an object at this point, since it has no external dependencies. Then it can also hold just a single instance of the API implementation, so that not every network call needs to recreate that either by calling into Retrofit.

object APIService {
    private val retrofit = Retrofit.Builder()
        .baseUrl("...")
        .addConverterFactory(GsonConverterFactory.create())
        .build()

    val pokemonService: PokemonService = retrofit.create()
}

0f4d3ff

---

We’ve discussed the PokemonColorUtil class before, let’s take a look at the other method inside it now:

fun getPokemonColor(typeOfPokemon: List<String>?): Int {
    val type = typeOfPokemon?.elementAtOrNull(0)
    val color = when (type?.toLowerCase()) {
        "grass", "bug" -> R.color.lightTeal
        "fire" -> R.color.lightRed
        ...
        else -> return R.color.lightBlue
    }
    return convertColor(color)
}

This method is using a when expression rather nicely to map the various tags that can be on a pokemon to a color that should be displayed. Notice however, that the expression first selects a color resource value, and then uses convertColor to get the actual ARGB color value for that resource.

On the else branch, however, there is a return call which doesn’t return that value from the when expression, running the rest of the function body, but instead returns from the outer function immediately! This means that in the default case, a color resource ID from R.java is returned (as it is an Int), while in the other cases, an actual ARGB color value (which is also technically an Int, so the type system doesn’t mind it).

If we add @ColorInt to the signature of the getPokemonColor method to signify what we mean to return, we actually get a compilation error:

To fix this, we simply remove the return keyword, and let the convertColor method do its job in the default case as well:

fun getPokemonColor(typeOfPokemon: List<String>?): Int {
    val type = typeOfPokemon?.elementAtOrNull(0)
    val color = when (type?.toLowerCase()) {
        "grass", "bug" -> R.color.lightTeal
        "fire" -> R.color.lightRed
        ...
        else -> R.color.lightBlue // <- here!
    }
    return convertColor(color)
}

2bc8d9a

---

There are usages of let in the code where the incoming parameter of the lambda is not named, but is used by the implicit it name instead:

pokemon?.typeofpokemon?.elementAtOrNull(0).let {
    root.textViewType3.text = it
    if (it == null) {
        root.textViewType3.visibility = View.GONE
    }
}

This makes the code hard to read, as to understand what it means somewhere in the middle of this lambda, we first have to find which scope it’s coming from, and then figure out what its value is.

Naming this parameter well makes the code a lot clearer:

pokemon?.typeofpokemon?.elementAtOrNull(0).let { firstType ->
    root.textViewType3.text = firstType
    if (firstType == null) {
        root.textViewType3.visibility = View.GONE
    }
}

96a246b

The usage of elementAtOrNull in the project is kind of odd, getOrNull is a more well-known method for the same operation.

053a1c4

If this code is ever ran more than once, it would also be a good idea to set the visibility of the View whether or not the current type is null (for example, using isVisible):

pokemon?.typeofpokemon?.elementAtOrNull(0).let { firstType ->
    root.textViewType3.text = firstType
    root.textViewType3.isVisible = firstType != null
}

12b51db

---

Now, let’s look at the following code from DashboardFragment with a focus on null handling:

arguments?.getString("id").let {
    dashboardViewModel.getPokemonById(it)
    // Logic setting up UI based on the result of the previous call
}

If arguments is null, then thanks to the safe call operator ?., the entire expression arguments?.getString("id") will evaluate to null as well. This means that inside let, the parameter referred to as it will be null too. We’re attempting to search for an item that has an ID of null.

This is probably something that will never yield a reasonable result, and we could avoid it by performing a null check on the ID on this level, and not propagating this invalid value further into our code. For example, we can throw an exception if we didn’t get an ID, because otherwise opening this screen wouldn’t make sense:

val id: String = arguments?.getString("id") 
            ?: throw IllegalStateException("We should have an ID here")

If this happens, it’s not an unexpected runtime error, it’s a mistake we’ve made in our code (not opening this Fragment correctly), and it should crash the application.

Depending on the semantics we want, we can also use the checkNotNull or requireNotNull methods here (see also this Twitter discussion about these methods):

val id: String = checkNotNull(arguments?.getString("id"))

d380051

---

One last bit about Kotlin usage:

There are a couple @JvmStatic annotations in the code, for newInstance methods. Not sure if these are there on purpose or just a leftover from a Java to Kotlin conversion, but they actually improve the performance of the code, by avoiding the allocation of a Companion object. Interesting!

Room

The PokemonDAO interface declares a getter function, which gets the details of a single Pokemon:

@Query("SELECT * FROM pokemon WHERE id = :id")
fun getById(id: String?): LiveData<List<Pokemon?>?>

Whenever this is used, the LiveData is observed, and the list’s first element is taken in the observer:

dashboardViewModel.getPokemonById(it).observe(this, Observer { list ->
    list?.get(0).let { pokemon -> 
        root.textViewID.text = pokemon?.id
        root.textViewName.text = pokemon?.name
        ...
    }
}

Since the expectation is that there will only ever be at most one Pokemon in the database for the given ID, returning a List is not necessary, we can instead ask for a single value from Room:

@Query("SELECT * FROM pokemon WHERE id = :id")
fun getById(id: String?): LiveData<Pokemon>

595f1e4

We can also clean up the call site some more, by using the ?.let {} construct instead of the regular .let {} call that we’ve had before. This way, the pokemon parameter received in the lambda passed to let will become non-null, as for a null value, the entire let block would’ve been skipped. This gets rid of all null handling within the let block, which contains a lot of safe calls (?.).

dashboardViewModel.getPokemonById(it).observe(this, Observer { poke ->
    poke?.let { pokemon ->
        root.textViewID.text = pokemon?.id
        root.textViewName.text = pokemon?.name
        ...
    }
}

af08dab

---

Next, let’s take a look at the add method of the Dao:

@Insert(onConflict = OnConflictStrategy.IGNORE)
fun add(vararg pokemon: Pokemon)

First, we should consider whether ignoring a new row that conflicts with an existing record is the correct thing to do here. Calls to this add method will get fresh data from the network as their parameter, so perhaps it would be more appropriate to overwrite the existing records in the database on conflict, using OnConflictStrategy.REPLACE.

3ebd4a2

If we look for the usage of this add method, we find this inside a Retrofit callback:

response?.body()?.let {
    val pokemons: List<Pokemon> = it
    Thread(Runnable {
        for (pokemon in pokemons){
            pokemonDAO.add(pokemon)
        }
    }).start()
}

The pokemons local variable simply renames the incoming parameter of the lambda, which we can also achieve by renaming that parameter directly. If we want to keep the explicit type for readability, we can do that too:

response?.body()?.let { pokemons: List<Pokemon> ->
    Thread(Runnable {
        for (pokemon in pokemons){
            pokemonDAO.add(pokemon)
        }
    }).start()
}

Then, in a new thread, pokemons are inserted one by one into the database. The add method takes a vararg argument - any number of values - but this is not used, and it’s invoked separately for each entity instead. This results in many calls to the database instead of just the one that’s necessary.

We can either fix this by spreading the arguments into the vararg with the spread operator, which unfortunately only works on arrays:

pokemonDAO.add(*pokemons.toTypedArray())

Or better yet, we can modify the add method to take a List<Pokemon> as its parameter:

@Insert(onConflict = OnConflictStrategy.REPLACE)
fun add(pokemon: List<Pokemon>)

On the call site, where we can now pass in the List directly, we can also use the thread function to fire off a new thread in a concise way.

response?.body()?.let { pokemons: List<Pokemon> ->
    thread {
        pokemonDAO.add(pokemons)
    }
}

07afada

Small tidbits

• There are a couple occurrances of fragmentManager!! in the code. The requireFragmentManager method does effectively the same thing, but makes the intent clearer, and makes for a nicer exception if it happens to crash. requireContext is a similarly handy method. 9edc0fc
• Whenever a list of data is observed from LiveData in the application, the Observer creates a new adapter that it places the data in, which it attaches to the RecyclerView (for example, in HomeFragment). It would be nicer to have a single adapter instance attached to the RecyclerView, and change the data inside that adapter.

Layouts and UI

Best for last! This is what people liked about this app on first glance, and they were right! The design (Saepul Nahwan) is really is cool, and in my opinion, implemented well here for native Android.

What I especially like in the implementation is the generous usage of tools: tags in the layout code, which makes the navigation editor look quite pretty for the app:

tools:itemCount and tools:listitem are some very cool things to have added here, as they make for great previews of screens like this:

Conclusion

Well, that’s it for this project, I hope you learned something along the way! I really hope the completed project will eventually stand as a good example of both Android design and Jetpack library usage.

Did you find this review useful? Do you have a project for me to review? Was this utterly useless for you? You can let me know about any of these things on Twitter!

And again, if you want to check out the project after applying all these changes, I encourage you to visit - and perhaps star 🌟 - the fork’s repository. Also check out the pull request that contains these changes!

Last but not least, thanks to Gabor Varadi (@zhuinden) who reviewed all of this for me!