Modifying code to make it more testable has benefits beyond testability. In general, code that is more testable

• Is easier to maintain,
• Is easier to reason about,
• Is more loosely coupled, and
• Has a better overall design, architecturally.

There are (seemingly) opposing forces in play.

• On the one hand, you want to enforce encapsulation
• On the other hand, you want to be able to test the software

Proponents of keeping all ‘implementation details’ private are usually motivated by the desire to maintain encapsulation. However, keeping everything locked down and unavailable is a misunderstood approach to encapsulation. If keeping everything unavailable was the ultimate goal, the only true encapsulated code would be this:

static void Main(string[] args)

Is your colleague proposing to make this the only access point in your code? Should all other code be inaccessible by external callers?

Hardly. Then what makes it okay to make some methods public? Isn’t it, in the end, a subjective design decision?

Not quite. What tends to guide programmers, even on an unconscious level, is, again, the concept of encapsulation. You feel safe exposing a public method when it properly protects its invariants.

I wouldn’t want to expose a private method that doesn’t protect its invariants, but often you can modify it so that it does protect its invariants, and then expose it to the public (of course, with TDD, you do it the other way around).

Opening up an API for testability is a good thing, because what you’re really doing is applying the Open/Closed Principle.

If you only have a single caller of your API, you don’t know how flexible your API really is. Chances are, it’s quite inflexible. Tests act as a second client, giving you valuable feedback on the flexibility of your API.

Thus, if tests suggest that you should open up your API, then do it; but maintain encapsulation, not by hiding complexity, but by exposing complexity in a fail-safe manner.

Looks like you’re talking about dependency injection. It’s really common, and IMO, pretty necessary for testability.

To address the broader question of whether it’s a good idea to modify code just to make it testable, think of it this way: code has multiple responsibilities, including a) to be executed, b) to be read by humans, and c) to be tested. All three are important, and if your code doesn’t fulfill all three responsibilities, then I’d say it’s not very good code. So modify away!

It’s a bit of a chicken and egg problem.

One of the biggest reasons why it’s good to have good test coverage of your code is that it allows you to refactor fearlessly. But you’re in a situation where you need to refactor the code in order to get good test coverage! And your colleague is fearful.

I see your colleague’s point of view. You have code which (presumably) works, and if you go and refactor it – for whatever reason – there’s a risk that you’ll break it.

But if this is code which is expected to have ongoing maintenance and modification, you’re going to be running that risk every time you do any work on it. And refactoring now and getting some test coverage now will allow you take that risk, under controlled conditions, and get the code into better shape for future modification.

So I would say, unless this particular codebase is fairly static and not expected to have significant work done in the future, that what you want to do is good technical practice.

Of course, whether it’s good business practice is a whole ‘nother can of worms..

This may just be a difference in emphasis from the other answers, but I’d say that the code should not be refactored strictly to improve the testability. Testability is highly important for maintenance, but testability is not an end in itself. As such, I would defer any such refactoring until you can predict that this code will need maintenance to further some business end.

At the point that you determine that this code will require some maintenance, that would be a good time to refactor for testability. Depending on your business case, it may be a valid assumption that all code will eventually require some maintenance, in which case the distinction I draw with the other answers here (e.g. Jason Swett’s answer) disappears.

To sum up: testability alone is not (IMO) a sufficient reason to refactor a code base. Testability has a valuable role in allowing maintenance on a code base, but it is a business requirement to modify your code’s function that should drive your refactoring. If there is no such business requirement, it’d probably be better to work on something your customers will care about.

(New code, of course, is being actively maintained, so it should be written to be testable.)

Your problem here is that your test tools are crap. You should be able to mock out that object and call your test method without changing it – because while this simple example is really simple and easy to modify, what happens when you have something a lot more complicated.

A lot of people have modified their code to introduce IoC, DI and interface-based classes simply to enable unit testing using the mocking and unit test tools that require these code changes. I don’t thin they’re a healthy thing, not when you see code that was quite straight-forward and simple turn into a nightmare mess of complex interactions driven entirely by the need to make each and every class method totally decoupled from everything else. And to add insult to injury, we then have many arguments on whether private methods should be unit tested or not! (of course they should, what’s the point of unit testing if you only test part of your system) but these arguments are more driven from the point of view that its difficult to test those methods using the existing tools – imagine if your test tool could run tests against a private method as easily as a public one – everyone would be testing them without complaint.

The problem, of course, is in the nature of the test tooling then.

There are better tools available now that could put these design changes to bed forever. Microsoft has Fakes (nee Moles) that allows you to stub concrete objects, including static ones, so you no longer need to change your code to fit the tool. In your case, if you used Fakes, you’d replace the GetTypes call with your own that returned valid and invalid test data – which is pretty important, your suggested change doesn’t provide for that at all.

To answer: your colleague is right, but possibly for the wrong reasons. Do not change code to test, change your test tool (or your whole test strategy to have more integration-style unit tests instead of such fine-grained testing).

Martin Fowler has a discussion around this area in his article Mocks aren’t Stubs

I think your colleague is wrong.

Others have mentioned the reasons why this is a good thing already, but so long as you are given the go ahead to do this, you should be fine.

The reason for this caveat is that making any change to code comes at the cost of the code having to be re-tested. Depending what you do, this test work may actually be a large effort by itself.

It is not necessarily your place to make the decision of refactoring versus working on new features which will benefit your company/customer.

I’ve used code coverage tools as part of unit testing in checking whether all paths through the code are exercised. As a pretty good coder/tester on my own, I usually cover 80-90% of the code paths.

When I study the uncovered paths and make an effort for some of them, that’s when I discover bugs such as error cases that will “never happen”. So yes, modifying code and checking for test coverage makes for better code.

There are some serious differences between your examples. In the case of DoSomethingOnAllTypes(), there is an implication that do something is applicable to types in the current assembly. But DoSomething(Assembly asm) explictly indicates that you can pass any assembly to it.

The reason I point this out is that a lot of dependency-injection-for-testing-only steps outside the bounds of the original object. I know you said “not ‘making everything public’“, but that’s one of the biggest errors of that model, followed closely by this one: opening the object’s methods up to uses they aren’t intended for.

A good and common practice is to use Unit testing and debug logs. Unit tests ensures that if you make any further changes in program, your old functionality is not breaking. Debug logs can help you trace the program at run time.
Sometimes it happens that even beyond that we need to have something only for testing purpose. It is not unusual to change the code for this. But the care should be taken such that the production code is not affected because of this. In C++ and C this is achieved using the MACRO, which is compile time entity. Then the testing code do not come into picture at all in production environment. Don’t know if such provision is there in C#.
Also when you add testing code in your program, it should be clearly visible that this portion of code is added for some testing purpose. Or else the developer trying to understand the code is simply going to sweat over that part of code.

Your question gave not much context in which your colleague argued so there is room for speculation

“bad practice” or not depends on how and when the changes are made.

In my opition your example to extract a method DoSomething(types) is ok.

But i have seen code which is not ok like this:

public void DoSomethingOnAllTypes()
{
  var types = (!testmode) 
      ? Assembly.GetExecutingAssembly().GetTypes() 
      : getTypesFromTestgenerator();

  foreach (var currentType in types)
  {
     if (!testmode)
     {
        // do something with this type that made the unittest fail and should be skipped.
     }
     // do something with this type (e.g: read it's attributes, process, etc).
  }
}

These changes made the code more difficuilt to understand because you increased the number of possible code-paths.

What i mean with how and when:

if you have a working implementation and for the the sake of “implementing testing-capabilities” you made the changes then you have to re-test you application because you may have broken your DoSomething() method.

The if (!testmode) is more more difficulilt to understand and test than the extracted method.