Maintainable Architecture – Introduction

Creating a maintainable, flexible codebase is not easy but is an essential part of software engineering. In this series we’ll take a look at a simple, functional weather app and look at some of the issues in its design. We shall then refactor and re-design it to create a codebase which will be easier to maintain, less prone to bugs, and easier to add features to.

Weather Station is a simple weather reporting app which displays the most recently observed weather data at the current location of the device. It uses the OpenWeatherApp API to obtain the weather data, retrieves it using Retrofit, and decodes the JSON using Moshi. The app is actually pretty stable and well behaved (for example, it cancels any outstanding Retrofit calls at an appropriate point in the Fragment lifecycle) and it is easy to conclude that the code is well organised and maintainable. However, that is not, in fact, the case and there is an awful lot that we can do to improve things.

The code consists of an Application instance which is used to initialise the ThreeTenABP library which is used for date time handling; A single Activity which handles obtaining runtime permissions for Location, and is the host for three separate Fragments; The first Fragment (NoPermissionFragment) will be displayed when the Location permission has not been granted; the second (CurrentWeatherFragment) displays the current weather data; and the third (PreferencesFragment) is a Preferences page which allows the user to specify the desired units for wind speed and temperature. Then there are some data classes (in the com.stylingandroid.weatherstation.model package) which are how the weather data gets represented as JVM objects, and the Retrofit HTTP API interface in com.stylingandroid.weatherstation.OpenWeatherMap. Finally there is a Converter class which takes a numeric value and converts it to a string using the units that the user has specified in preferences.

The big problem area is CurrentWeatherFragment. While this isn’t a huge class (it’s only around 200 lines of code including imports), there is instantly a smell about it before the class declaration begins:

package com.stylingandroid.weatherstation.ui

import android.annotation.SuppressLint
import android.content.Context
import android.os.Bundle
import android.view.LayoutInflater
import android.view.Menu
import android.view.MenuInflater
import android.view.MenuItem
import android.view.View
import android.view.ViewGroup
import androidx.appcompat.app.AppCompatActivity
import androidx.core.location.component1
import androidx.core.location.component2
import androidx.fragment.app.Fragment
import androidx.fragment.app.transaction
import androidx.transition.TransitionManager
import com.google.android.gms.location.FusedLocationProviderClient
import com.google.android.gms.location.LocationCallback
import com.google.android.gms.location.LocationRequest
import com.google.android.gms.location.LocationResult
import com.google.android.gms.location.LocationServices
import com.squareup.moshi.Moshi
import com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterFactory
import com.stylingandroid.weatherstation.BuildConfig
import com.stylingandroid.weatherstation.Converter
import com.stylingandroid.weatherstation.R
import com.stylingandroid.weatherstation.model.Current
import com.stylingandroid.weatherstation.net.OpenWeatherMap
import kotlinx.android.synthetic.main.fragment_current_weather.*
import okhttp3.Cache
import okhttp3.OkHttpClient
import okhttp3.logging.HttpLoggingInterceptor
import org.threeten.bp.LocalDateTime
import org.threeten.bp.ZoneId
import org.threeten.bp.format.DateTimeFormatter
import org.threeten.bp.format.FormatStyle
import retrofit2.Call
import retrofit2.Callback
import retrofit2.Response
import retrofit2.Retrofit
import retrofit2.converter.moshi.MoshiConverterFactory
import java.util.Locale

Just the sheer number of imports is an issue – it indicates that this Fragment has many collaborators. It is also with considering the package name – it identifies that this is part of the UI, yet the imports suggest that it is touching lots of other areas as well – such as Retrofit (which is handling the network call), and the location provider. We would expect it to only have access to the actual data produced by those, and not collaborate with them directly. As it stands this is a clear violation of the single responsibility principle (part of SOLID), and also we do not have a good separation of concerns. While this may not appear to be an issue in a small app such as Weather Station, we will quickly start running in to problems if we start adding features. For example, perhaps we may want to add the 5-day forecast which is also available from OpenWeatherMap, then we would need to start reproducing the code from here to wherever we obtain the 5-day forecast, which may be in a different Fragment.

Another problem here is that there is no degree of abstraction from these collaborators. What if we wanted to move to a different weather data provider? By having this logic inside all of our Fragments it is a major undertaking to replace all of these to use the new provider. Or alternatively, we may want to switch from Retrofit to another HTTP client library because new features demand it – once again this would be difficult because of the close collaboration with Retrofit in this Fragment. And these complexities will only get worse as our codebase grows – and we have lots of replicated code, which is tightly-coupled to Retrofit and OpenWeatherMap, and replacing either or both of these would become a major undertaking.

That is not the only concern here. Although the app works, it is actually pretty inefficient because it will request the data each time the Fragment is loaded, including when the device is rotated causing the Activity and Fragment to be re-created. It will also retrieve new data when the user goes to the settings page and then returns to the current forecast page. Constantly retrieving new data in this way forces the user to wait while new data is retrieved, but it is also consuming network bandwidth and battery which is bad news for the user. It may also be making lots of API calls and if the service we are using has limits (the free OpenWeatherMap API permits no more than 60 API calls per minute) then we may find that multiple users using the app results in our API call limit being consumed extremely quickly.

Those familiar with Retrofit / OkHttp may think that enabling a local cache may overcome this issue by serving up cached data. However the OpenWeatherMap API does not include eTag or expiry headers in its responses, so conditional GETs are not supported. A local cache will only work unaided if the server supports conditional GET requests. To protect against this I have implemented a local cache in the OkHttpClient construction:

class CurrentWeatherFragment : Fragment() {

    private val cacheSize: Long = 10 * 1024 * 1024

    private var fusedLocationProviderClient: FusedLocationProviderClient? = null
    private var call: Call<Current>? = null

    private var currentWeather: Current? = null
    private lateinit var converter: Converter

    private val okHttpClient: OkHttpClient by lazy {
        context?.let {
            OkHttpClient.Builder()
                    .cache(Cache(it.cacheDir, cacheSize))
                    .addInterceptor(HttpLoggingInterceptor().apply {
                        level = HttpLoggingInterceptor.Level.BODY
                    })
                    .build()
        } ?: throw IllegalStateException("Context is not valid")
    }
    ...
}

Each API call will have a 10 minute expiry which is specified using a cache-control header:

interface OpenWeatherMap {

    @GET("/data/2.5/weather")
    @Headers("Cache-Control: private, max-age=600, max-stale=600")
    fun currentWeather(
            @Query("lat") latitude: Double,
            @Query("lon") longitude: Double,
            @Query("appid") appId: String
    ): Call<Current>
}

This will effectively throttle the number of API calls to a maximum of one every 10 minutes. Any calls made within that 10 minutes will be served up a locally cached version.

A further issue here is how the navigation within the app is organised. Each Fragment is responsible for handling the navigation when it is active. Once again it will become more and more complicated by having this navigation logic dotted throughout the codebase, and is likely to result in inconsistent behaviour throughout the app – again the user will suffer as a result.

The other really important thing to bear in mind that much of our app logic is bundled in here and this class is really difficult to test. It is responsible for creating the Retrofit instance used to retrieve the data, so that is difficult to mock using this design. There is one small part of the app which is designed much better – the Converter class:

class Converter(
        val context: Context,
        private val sharedPreferences: SharedPreferences =
                PreferenceManager.getDefaultSharedPreferences(context)
) {

    fun speed(value: Float): String =
            sharedPreferences.getString("Speed", "mph").let { units ->
                when (units) {
                    "mph" -> msToMph(value)
                    else -> value
                }
            }.let { newValue ->
                context.getString(R.string.wind_speed, newValue)
            }

    fun temperature(value: Float): String =
            sharedPreferences.getString("Temperature", "celsius").let { units ->
                when (units) {
                    "celsius" -> kelvinToCelsius(value) to R.string.temperature_celsius
                    "fahrenheit" -> kelvinToFahrenheit(value) to R.string.temperature_fahrenheit
                    else -> value to R.string.temperature_kelvin
                }
            }.let { (newValue, template) ->
                context.getString(template, newValue)
            }


    private fun kelvinToCelsius(value: Float): Float = value - 273.15f
    private fun kelvinToFahrenheit(value: Float): Float = (9f / 5f * (value - 273.15f)) + 32
    private fun msToMph(value: Float): Float = value * 2.2369362920544f

}

Although it is really quite small and simple, it provides a lot of power in a small package because it removes and need for CurrentWeatherFragment to touch SharedPreferences, or even be concerned about the units that the user has selected as their preference. Moreover, the only real collaborator is the SharedPreferences instance, and this is obtained via a default constructor argument (line 3-4). Kotlin’s default constructor arguments can really aid us in making our classes more testable. This allows us to substitute in a mock SharedPreferences instance and therefore permits us to create a test suite with good coverage

So the problems are many, but in a simple app such as this they are pretty easy to resolve, and we’ll tackle this using a variety of techniques and we progress. In the next article we’ll look at how we can begin fixing things up.

The source code for this article is available here.

© 2018, Mark Allison. All rights reserved.

Copyright © 2018 Styling Android. All Rights Reserved.
Information about how to reuse or republish this work may be available at http://blog.stylingandroid.com/license-information.