I got some great comments on the post, and I answered a few in comments but one started to get very long-winded so I decided to convert my response into a post.
Integration tests before refactoring
The first question is around whether it would be a good idea to write an integration test around code before refactoring.
I hate integration tests. It may be the kinds of teams that I've been on, but in the majority of cases, they:
- Were very expensive to write and maintain
- Took a long time to run
- Broke often for hard-to-determine reasons (sometimes randomly)
- Didn't provide useful coverage for the underlying features.
Typically, the number of issues they found was not worth the amount of time we spent waiting for them to run, much less the cost of creating and maintaining them.
There are a few cases where I think integration tests are justified:
- If you are doing something like ATDD or BDD, you are probably writing integration tests. I generally like those, though it's possible they could get out of hand as well.
- You have a need to maintain conformance to a specification or to previous behavior. You probably need integration tests for this, and you're just going to have to pay the tax to create and maintain them.
- You are working in code that is scary.
"Scary" means something very specific to me. It's not about the risk of breaking something during modification, it's about the risk of breaking something in a way that isn't immediately obvious.
There are times when the risk is significant and I do write some sort of pinning tests, but in most cases the risk does not justify the investment. I am willing to put up with a few hiccups along the way if it avoids a whole lot of time spent writing tests.
I'll also note that to get the benefit out of these tests, I have to cover all the test cases that are important. The kinds of things that I might break during refactoring are the same kind of things I might forget to test. Doing well at this makes a test even more expensive.
In the case in the post, the code is pretty simple and it seemed unlikely that we could break it in non-obvious way, so I didn't invest the time in an integration test, which in this case would have been really expensive to write. And the majority of the changes were done automatically using Resharper refactorings that I trust to be correct.
Preserving the interface while making it testable
This is a very interesting question. Is it important to preserve the class interface when making a class testable, or should you feel free to change it? In this case, the question is whether I should pull the creation of the LegacyService instance out of the method and pass it in through the constructor, or instead use another technique that would allow me to create either a production or test instance as necessary.
Let me relate a story…
A few years ago, I led a team that was responsible for taking an authoring tool and extending it. The initial part had been done fairly quickly and wasn't very well designed, and it had only a handful of tests.
One day, I was looking at a class, trying to figure out how it worked, because the constructor parameters didn't seem sufficient to do what it needed to do. So, I started digging and exploring, and I found that it was using a global reference to an uber-singleton that give it access to 5 other global singletons, and it was using these singletons to get its work done. Think of it as hand-coded DI.
I felt betrayed and outraged. The constructor had *lied* to me, it wasn't honest about its dependencies.
And that started a time-consuming refactoring where I pulled out all the references to the singletons and converted them to parameters. Once I got there, I could now see how the classes really worked and figure out how to simplify them.
I prefer my code to be honest. In fact, I *need* it to be honest. Remember that my premise is that in TDD, the difficulty of writing tests exerts design pressure and that in response to that design pressure, I will refactor the code to be easier to test and that aligns well with "better overall". So I am hugely in preference of code that makes dependencies explicit, both because it is more honest (and therefore easier to reason about), and because it's messy and ugly and that means I'm more likely to convert it to something that is less messy and ugly.
Or, to put it another way, preserving interfaces is a non-goal for me. I prefer honest messiness over fake tidiness.