Refactoring Legacy Code: Leave Tested Methods Public

When refactoring from a large to small API, use interfaces for the new methods, but leave the old public IFF they are well tested. Its not worth re-working all the tests when they're hidden by using the interface

This is a short little post about a small thing I did in a legacy codebase at work that I'd never do in greenfield work.
A class has a public method that isn't in the interface!!! NOOOOOOOOOOO!!!!!!

Anyway... As you refactor legacy code you'll often change the API so that previously public things may no longer be needed public. This happened when I was moving responsibilities around and some public methods no longer got invoked outside of the class. It became a single method call.

void Foo(IBar bar){
    var res = bar.Do();
    res = bar.The(res);
    res = bar.Thing(res);

While not QUITE that clear, it was a required sequence of calls, with other code interspaced in there.
This was simplified to a single method on IBar, DoTheThing. (Like how I named that?)

void Foo(IBar bar){
    var res = bar.DoTheThing();

Internal to the concrete type implementing IBar it invokes the previous methods

class Bar : IBar{
    IRes DoTheThing(){

(ignoring other details).

Assuming we've got a nice suite of tests around the original multi-method class Bar ... that's a lot of re-writing tests to cover all the cases. I don't recommend it.

There's a couple reasons I don't like to re-write tests. Are you SURE, 100%, NO DOUBT, that you've faithfully written a NEW test that validates EVERYTHING the previous test(s) did?
It's few and far between I trust myself that much with this type of code to write a new tests and not miss something the old one was doing. The new tests are going to become more complex, more opportunities to screw it up and REDUCE the confidence we have for future refactors. We don't want that.

I leave the methods public.

We take them out of the interface. That's a huge win when we have the interface - we aren't constrained to the class definition. We aren't exposing functionality the class has exposed. As long as you avoid trying to cast, we'll be OK. If you're doing type inspection or casting, you'd doing it wrong already and we need to have a chat.

I leave the tests in place.

In the refactor I'm doing, we're not changing the behavior of the class; just simplifying it for the consumer. Which makes future refactors of this object much easier.
Shove the complexity down. Own the complexity instead of your clients.

In the MicroObjects land, this makes it much easier to extract classes from the contained behaviors and do so as a black box to the rest of the system.

As I picked up From A Philosophy of Software Design - This makes is so your module has a narrow interface but deep functionality. The result of MicroObjects technical practices has this effect. It's great.

Quick and Simple

It's a quick and simple way to do less and continue to improve the maintainability of the system. Leave methods previously part of the interface public to keep the existing tests. When you simplify the exposed API, that doesn't mean we need to change everything; we're just changing what the client/consumer sees. The tests for the class should be kept around as-is.

Show Comments