Good design for a class with multiple methods to test but one public method

Question

I've been trying to refactor some existing code which is in essence a giant nested procedural call inside what should otherwise be an object oriented architecture. The entry point to the relevant code is a static function, which iteslf calls other static functions calling other static functions etc. totaling to a few thousand lines of code. At a high level, the calls perform two actions of

1. Performing metadata updates prior to real work
2. Executing work related to the call

For the metadata updates, it can be further subdivided into 3 sections which are relatively granular

1. Query for some data related to the overall call
2. Use information from step 1 to update physical file level metadata
3. Use information from step 1 to perform a metadata update at a higher abstraction level than step 2

Originally I had created a class to handle all of the metata updates, with 3 private methods for each of the work. Roughly the following code

class Metadata(...) {
  def updateOnEvent(...): Unit = {
    val data = queryFromStep1()

    if (data.requiresUpdating) {
      updatePhsicalFileMetadata(data)
    }
    updateHigherLevelMetadata(data)
  }

  private def queryFromStep1(): Data = ???

  private def updatePhsicalFileMetadata(data: Data): Unit = ???

  private def updateHigherLevelMetadata(data: Data): Unit = ???
}

The issue I'm having is that I feel the urge to unit test the 3 private methods here, and my understanding is that this is a sign of a poor class abstraction.

My next thought was to extract each method into a separate class, and test each class individually, in addition to tests for the Metadata class to make sure everything is glued together properly. However this doesn't seem to be a great solution either since now we have many tiny classes each exposing only a single public method, with no private methods either.

I'm wondering what a better solution could possibly be. I'm hesitant to expose all 4 methods as public from the Metadata class, because the caller really only cares about updateOnEvent for the purpose of handling an event. Is there a simple solution I'm missing? I can provide more details if needed.

Answer 1

Why did you make these methods private in the first place? Hopefully you wanted to hide implementation details. Doing so helps keep the relationship between this class and code that uses it simple. That is to say, decoupled.

If you let test code sneak past that public method then the test code defeats that attempt at decoupling. Let that happen and refactoring becomes a huge pain.

My next thought was to extract each method into a separate class, and test each class individually, in addition to tests for the Metadata class to make sure everything is glued together properly. However this doesn't seem to be a great solution either since now we have many tiny classes each exposing only a single public method, with no private methods either.

Tiny classes are a good thing. There is no rule that a class must have private methods.

But private methods are not the only way to hide an implementation. You can use one object to hide many other objects. The facade pattern does exactly this. That still hides the implementation so long as using code respects the pattern and uses the object that offers that simplified interface. That abstraction needs to be tested. Testing it documents it.

Now the objects behind that facade object can have their own public methods. And you can write code that tests those public methods. Why is this OK when they are the same methods that used to be private?

Because now the code that tests the facade objects interface (an abstraction) and code that pokes around in it's implementation are clearly separated. This makes it easier to delete the tests when you delete the methods/classes that go with them. Something you can do with confidence only because you have good tests on the facade.

The danger here is that you start to care about code that you never use. Code that is tested when the facade is tested you know can get used. Code that only gets called by tests that reach behind the facade might never be needed and will clutter your code base.

The lesson here is that the structure of your code doesn't tell you what to test. After all, you can just restructure your code. You should test an abstraction. When you reach behind that abstraction you are not supporting it. You may be supporting another abstraction that has a good reason to be tested on it's own. But that doesn't help the one you reached past at all.

So it comes down to whether you have a real need to support more than the one abstraction. Sometimes you do. Abstractions can nest. But remember, I'm just a mere mortal. Don't make it complicated if you don't need to. Or you'll just end up confusing me.

Answer 2

Have a look at your development software first. Some have an option to mark a function as “testable” which means it may be private and usually no accessible from the outside, but the compiler makes an exception when compiling unit tests.

If you can only test public methods, and you have methods that you want to be private and that you want to test, then something must give. You can make the method public even though it shouldn’t be called from the outside. You can not test it. You can rename the function from “someFunction” to “someFunctionPublicForTestingOnly“. The last one is ugly but works.

Now some people will tell you to test only the public method. That’s misguided. If the actions of your public method are complex enough that you need to split it up, and the parts are difficult enough to contain possible bugs, then you should unit test these methods. Next they tell you to split up your class - so your private methods are pointlessly exposed to the world.