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:
By introducing a name for an operation, the code becomes more self-documenting.
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) {
throw new TreeException("Value not found in tree!");
}
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) {
throw new TreeException("Value not found in tree!");
}
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.
Best Answer
You should extract code into its own class if it is conceivable that it would be called by code other than the current caller, in particular by code that doesn't do essentially the same thing.
A perfect example for code that should be independent would be code that simply constructs the days of the week. Many conceivable callers could use this, therefore it is a good idea to factor it out even if there is actually only one caller and this will probably not change. The reason is simply that it is easier to think about only one task at a time: either date calculation or parking logic.
If your code does date manipulation and parking business logic it is less clear that this is functionality that could be used differently. You know better than we whether this is the case, but it might be.
A counterexample is a routine that performs complicated calculations on a line item in a complicated workflow (e.g. declaring taxes, verifying space shuttle code). Such special-purpose code is almost certainly of no use outside the context it is now in. Such code should certainly go into its own method, but a separate class is often overkill.