I recently reviewed some code at work that made me pause. The change affected a part of the codebase that we affectionately call “The Monolith”, which is some of the oldest code in our 16 year old app. While there are reasonable arguments that it’s hard to tease apart this older hairball of code, there are often small choices that can either move you closer to the decoupled promised land, or further entrench you in bad patterns.

This change that I was reviewing was relatively simple. Our app’s main screen has a tab layout, which swaps content views when you switch tabs. For this particular change, the goal was to run an experiment in which we show an alternative content view for one of the tabs.

While this is a relatively straight-forward change, the tab host screen is some of our oldest code, having been a core part of our app for over a decade. Over that time it’s accumulated some bad patterns. This can be easy to ignore in isolation, but is the kind of place where a little proactive maintenance can stop these bad patterns from continuing throughout our codebase.

Constructing dependencies⌗

The first thing that stood out to me in this review was that the tab host screen was constructing the content Fragments. This is a pretty obvious place for improvement, as classes that directly construct their dependencies is a classic example of what dependency injection exists to fix.

For this experiment, using this pattern meant that the host screen had to have the feature gate injected, so it would know which content fragment to construct. This is an example of the feature gate, an implementation detail of the Home feature, being leaked into the monolith.

fun onTabSelected(tab: Tab) {
  var fragment: Fragment = when (tab) {
    FilesTab -> BrowserFragment.create(...)
    PhotosTab -> PhotosFragment.create(...)
    HomeTab -> {
      if (newHomeFeatureGate.isEnabled) {
        NewHomeFragment.create(...)
      } else {
        ModularHomeFragment.create(...)
      }
    }
  }
}

Using dependency injection, this kind of “alternative implementation” approach is quite simple. I covered it in detail in another post, but the basic idea is to inject a factory that can create the right Fragment based on the state of the feature gate.

// features/home
@Inject class HomeFragmentFactory(
  private val newHomeFeatureGate: NewHomeFeatureGate,
) {
  fun create(...): Fragment = if (newHomeFeatureGate.isEnabled) {
    NewHomeFragment.create(...)
  } else {
    ModularHomeFragment.create(...)
  }
}

// features/main
@Inject class MainScreen(
  private val homeFragmentFactory: HomeFragmentFactory,
) {

  fun onTabSelected(tab: Tab) {
    var fragment: Fragment = when (tab) {
      FilesTab -> BrowserFragment.create(...)
      PhotosTab -> PhotosFragment.create(...)
      HomeTab -> homeFragmentFactory.create(...)
    }
  }
}

With this simple change we’ve extracted a good deal of implementation detail from our monolith. The only public API that needs to be exposed from the home feature is the HomeFragmentFactory, helping our monolith get a little bit lighter.

Before recommending this change, however, I wanted to make sure there weren’t other uses of the specific Fragment classes within the main screen. Fortunately there were only a small number of other uses, falling into another familiar pattern.

Behavior modification⌗

At some point a new product requirement came in saying that content screens that are scrollable, like the file browser, should jump to the top if the user reselects the currently selected tab. This is good app behavior.

When this was initially implemented, there was probably only a single content view that required this behavior, so the engineer simply checked if that was the current content view and called the instance method to reset the scroll state. As this requirement was adopted by other content views, that type checking grew.

 fun onTabReselected(tab: Int) {
  if (currentTab is BrowserFragment) {
    currentTab.resetScrollState()
  } else if (currentTab is PhotosFragment) {
    currentTab.scrollToTop()
  } else if (currentTab is ModularHomeFragment) {
    currentTab.resetState()
  }
}

This isn’t ideal, as the specific implementation of these content Fragments are now leaked into the main screen. A classic symptom of this is the fact that if we ever change which Fragment is returned from the HomeFragmentFactory defined above, we also have to change the implementation of MainScreen to maintain behavior.

--- old/MainScreen.kt
+++ new/MainScreen.kt
 fun onTabReselected(tab: Int) {
   if (currentTab is BrowserFragment) {
     currentTab.resetScrollState()
   } else if (currentTab is PhotosFragment) {
     currentTab.scrollToTop()
   } else if (currentTab is ModularHomeFragment) {
     currentTab.resetState()
+  } else if (currentTab is NewHomeFragment) {
+    currentTab.resetState()
   }
 }

In isolation, given the complexity and age of the main screen, this seemed reasonable. The engineer’s scope of work was to create a new Fragment that could take the place of the old one, and he did just that. But it’s these kinds of isolated decisions that perpetuate bad practices and make codebases significantly harder to work with as time goes on. This is a perfect candidate for an incremental improvement.

While the methods that each of these content Fragments use to reset their scroll state are different, what the onTabReselected function is really interested in is if the current fragment can reset it’s scroll state. In order to do that, though, the function is checking what the current fragment is.

This is a classic example of leaked implementation details. This main screen has inside knowledge that BrowserFragment happens to support resetting it’s scroll state, so it can simply check if the current tab is a BrowserFragment. Imagine how much simpler and more flexible this would be if the main screen could simply ask the current tab if it supported resetting scroll state.

Extracting the behavior⌗

Interfaces are part of Object Oriented Programming 101, but often focus on what an object is. A Car is a Vehicle. A Dog is an Animal. But it’s important to remember that the purpose of this is to group common traits and behaviors. That is, the things an object has or can do.

With that in mind we can extract this common behavior, instances that are able to reset their scroll state, into a simple interface. To practice lazy naming, we can make this interface simply describe the behavior that it represents: CanResetScrollState.

interface CanResetScrollState {
  fun resetScrollState()
}

Now we can update any content fragments that support this behavior so that they implement our new interface and remove those implementation details from our MainScreen.

--- old/MainScreen.kt
+++ new/MainScreen.kt
 fun onTabReselected(tab: Int) {
-  if (currentTab is BrowserFragment) {
-    currentTab.resetScrollState()
-  } else if (currentTab is PhotosFragment) {
-    currentTab.scrollToTop()
-  } else if (currentTab is ModularHomeFragment) {
-    currentTab.resetState()
+  if (currentTab is CanResetScrollState) {
+    currentTab.resetScrollState()
   }
 }

With these simple changes in place, our MainScreen relies on fewer implementation details from other features. Changing other features becomes simpler, as we don’t have to update this old code whenever we make a change, and we can continue on the path of braking down our monolith.

Incremental maintenance⌗

The best part of these kinds of improvements is that they don’t have to be all-or-nothing. Remember that onTabSelected function that was constructing it’s own content fragments?

  fun onTabSelected(tab: Tab) {
    var fragment: Fragment = when (tab) {
      FilesTab -> BrowserFragment.create(...)
      PhotosTab -> PhotosFragment.create(...)
      HomeTab -> homeFragmentFactory.create(...)
    }
  }

While we took good steps toward eliminating some tight coupling by injecting the homeFragmentFactory, it’s still constructing the other fragments it needs directly. This certainly isn’t the ideal end state, but we were able to make incremental gains without adding significant risk or blowing our scope.

It’s this practice, which I call incremental maintenance, that allows software to age gracefully, making it easier to work with as time goes on. By keeping an eye out for these patterns, and making the small changes as they come up to move the codebase forward, you can eliminate the cost and risk of rewrites and make your code more enjoyable to work in no matter how old it is.