# How does breaking up a big method into smaller methods improve unit testability when the methods are all private?

I'm presently reading Building Maintainable Software by Joost Visser and some of the maintenance guidelines they recommend include: A) each unit/method should be short (less than 15 lines per method) and B) methods should have a low cyclomatic complexity. It suggests that both these guideline helps with testing.

The example below is from the book explaining how they would refactor a complex method to reduce the per method cyclomatic complexity.

Before:

``````public static int calculateDepth(BinaryTreeNode<Integer> t, int n) {
int depth = 0;
if (t.getValue() == n) {
return depth;
} else {
if (n < t.getValue()) {
BinaryTreeNode<Integer> left = t.getLeft();
if (left == null) {
} else {
return 1 + calculateDepth(left, n);
}
} else {
BinaryTreeNode<Integer> right = t.getRight();
if (right == null) {
} else {
return 1 + calculateDepth(right, n);
}
}
}
}
``````

After:

``````public static int calculateDepth(BinaryTreeNode<Integer> t, int n) {
int depth = 0;
if (t.getValue() == n)
return depth;
else
return traverseByValue(t, n);
}
private static int traverseByValue(BinaryTreeNode<Integer> t, int n) {
BinaryTreeNode<Integer> childNode = getChildNode(t, n);
if (childNode == null) {
} else {
return 1 + calculateDepth(childNode, n);
}
}
private static BinaryTreeNode<Integer> getChildNode(
BinaryTreeNode<Integer> t, int n) {
if (n < t.getValue()) {
return t.getLeft();
} else {
return t.getRight();
}
}
``````

In their justification they state (emphasis mine):

Argument:

“Replacing one method with McCabe 15 by three methods with McCabe 5 each means that overall McCabe is still 15 (and therefore, there are 15 control ow branches overall). So nothing is gained.”

Counter Argument:

Of course, you will not decrease the overall McCabe complexity of a system by refactoring a method into several new methods. But from a maintainability perspective, there is an advantage to doing so: it will become easier to test and understand the code that was written. So, as we already mentioned, newly written unit tests allow you to more easily identify the root cause of your failing tests.

Question: How does it become easier to test?

According to the answers to this question, this question, this question, this question and this blog we should not be testing private methods directly. Which means we need to test them via the public methods that use them. So going back to the example in the book, if we are testing the private methods via the public method, then how does the unit tests functionality improve, or change at all for that matter?

• That is a very good question. I suspect the answer is simply that the authors are talking about public functions or free pure functions so the problem doesn't arise, and for private methods it really doesn't help testability (though it does help plenty of other -abilities). Commented Jan 18, 2016 at 0:30
• It doesn't. However, the consequence is not "The authors are wrong and you shouldn't refactor." It's "You should refactor and make helper functions public". If you need many helper functions to achieve one module's task, almost always you can extract a more generic data processing helper module that is useful in itself, and should be tested separately. Commented Jan 18, 2016 at 7:39
• @Adrian773 is the emphasis on "it will become easier to test" yours or is it in the book ? Commented Jan 18, 2016 at 12:53
• @guillaume31 Yes, the emphasis is mine. Commented Jan 18, 2016 at 19:28

After having written lots of test, I am strongly in favour of splitting up large methods, and of testing private methods. Splitting up functionality into smaller steps has two great advantages:

1. By introducing a name for an operation, the code becomes more self-documenting.

2. By using smaller methods, the code is simpler and thus more likely correct. E.g. you can immediately understand `getChildNode()`.

While the overall program cyclomatic complexity isn't reduced, these two advantages outweigh the bit of extra code in my book. There is a third advantage: the code becomes much easier to test, assuming we can get around the `private` access modifier.

People who advise against testing private implementation details make a good point: the test should show that the implementation adheres to it's public interface, but this can only be done by black-box tests of the public methods. Such tests don't require that you get 100% coverage, but missing coverage is an indication of dead code that's not required by the specification. Since TDD tests should define externally observable behaviour, such tests fall into this category.

But you can also use a different approach to testing: showing that an existing implementation is likely correct, and works as the programmer expected. Since we already have the code, we can design our tests to maximize coverage. We can use boundary values to meticulously exercise the behaviour of the program. In other words, we can write white-box tests that know about implementation details of the system under test. These tests are highly coupled to the implementation, but that is OK as long as you also have more general black-box tests.

As a result, I prefer a few black-box tests that walk through the “happy path” and basic guarantees of the interface. But it is way to cumbersome to exercise all possibilities: with each input parameter or state variable in the function, the test space increases exponentially! A function `f(bool)` might take two tests, a function `f(bool, bool)` 2²=4, and `f(bool, bool, bool, bool)` already 24=16. This is untenable. But by splitting a large function into smaller functions, I only have to show that each smaller function works as expected, and that the functions work together correctly (I call this testing by induction). My workload now adds, instead of multiplying – a great improvement if you want to be thorough!

In your concrete example, either possibility is suboptimal because in the first try there's loads of code duplication requiring duplicate testing, and in the second try there are interdependencies between the functions that cannot be mocked away. Only `getChildNode()` is easy to test, but this function is incorrect if `n` is equal to `t.getValue()`, which will never happen if that function is only ever called by `traverseByValue()`. An easy to test alternative would be:

``````public static int calculateDepth(BinaryTreeNode<Integer> t, int n) {
if (t == null) {
}

if (t.getValue() == n) {
return 0;
}

BinaryTreeNode<Integer> child = null;
if (n < t.getValue()) {
child = t.getLeft();
} else {
child = t.getRight();
}

return 1 + calculateDepth(child, n);
}
``````

This specific case doesn't even use any helper functions, because it's simple enough to do without – there are only 4 paths through this code. Your previous implementation hid this by nesting conditionals when the other branch had already been terminated by a `throw` or `return`, and by unnecessary code duplication.

However, testing a recursion or loop can be difficult. While we could create a fairly complex tree and check for the correct result, we would like a way to check the loop invariant. In a language with higher-order functions, this might be:

``````public static int calculateDepth(
BinaryTreeNode<Integer> t,
int n)
{
return calculateDepthLoop(t, n, calculateDepthLoop);
}

type Recurser = int(BinaryTreeNode<Integer>, int, Recurser);

private static int calculateDepthLoop(
BinaryTreeNode<Integer> t,
int n,
Recurser recurse)
{
if (t == null) {
}

if (t.getValue() == n) {
return 0;
}

BinaryTreeNode<Integer> child = null;
if (n < t.getValue()) {
child = t.getLeft();
} else {
child = t.getRight();
}

return 1 + recurse(child, n, recurse);
}
``````

Now we could run a test plan like:

• `calculateDepthLoop(null, ANY, ANY)` throws.
• `calculateDepthLoop(Tree(x, ANY, ANY), x, ANY) is`0`.
• `calculateDepthLoop(Tree(x, left, ANY), y, callback)` for `y < x` invokes `result = callback(left, y, callback)` and returns the `result + 1`.
• `calculateDepthLoop(Tree(x, ANY, right), y, callback)` for `x < y` invokes `result = callback(right, y, callback)` and returns the `result + 1`.

with only 4 tests (one for each path) we can be sure that `calculateDepthLoop()` works as expected. We might want a couple more just to be sure that everything works for all valid values of `x` and `y`. Now we only need another test to check that everything integrates as it should, this can be done with a black-box test of `calculateDepth()`, which I'd do by creating a moderately complex tree requiring the function to recurse both left and right and return some value.

I can see 2 possibilities :

• The author is referring to private method names being included in the stack trace when a unit test fails unexpectedly.

Hence

as we already mentioned, newly written unit tests allow you to more easily identify the root cause of your failing tests.

What still puzzles me is newly written unit tests. What does he mean, and what is it that he already mentioned ?

• The author got carried away by how marvelous refactoring to private methods is and forgot that they're extremely difficult to test in isolation.

I suspect the latter. The other benefit he mentions (better understandability) remains valid though.

This is a very good question and has been bugging me for a while..

There are the following possibilities:

1. Split into private methods as shown, but then change the acceess modifiere from private to package protected and test each such method with its own unit test

2. Extract the methods in a static 'helper' class and mock that class with Powermockito , but also cover the methods in that static class with their respective unit tests

3. Extract the methods in a non static collection of methods in a separate class and mock that one with Mockito..

None of these seem to be working well because in case 1 you need to also test the public method and you end up preparing test data twice.. (once for the protected method and once for the public) - but this could be overcome with refactoring the unit test common functionality into unit test helper methods..

But from a maintainability perspective, there is an advantage to doing so: it will become easier to test and understand the code that was written.

it will become easier to understand the code

I think so far in this discussion that this point is more-or-less accepted.

So, the question becomes: If code becomes easier to understand, does it therefore become easier to test?

I would suggest that it obviously does, if for no other reason then it makes it easier to identify those inputs and conditions which are especially at risk for failure.

I would suggest not splitting into smaller bits, but just making the function itself smaller and more readable. In addition, I fixed the second argument type, and throw an exception if the argument t is null where the original code crashes. Plus no unnecessary function calls. Plus no code duplication. Plus one function to test only.

``````public static int calculateDepth(BinaryTreeNode<Integer> t, Integer n) {
for (int depth = 0; t != null; ++depth) {
Integer value = t.getValue();
if (n == value)
return depth;
t = n < value ? t.getLeft() : t.getRight();
}