How to refactor a method to make it easier to test

Question

Below is a method that I'm having a hard time figuring out how to test using JUnit.
This method is difficult to test because it depends on the results of other methods (e.g. getClosestDcoumentCode).

Based on my reading of JUnit, this suggests I should refactor the method. But how? And if refactoring is not necessary, how do you test a method that depends on other methods?

Thank you,

Elliott

private static String findPrincipal(List<DocumentKey> documentkeys_) {
    Hashtable<String, Integer> codecounts = new Hashtable<String, Integer>();
    for (DocumentKey document : documentkeys_) {
        int x = 0;
        String closestCode = getClosestDocumentCode(document.candidates);
        if (closestCode == null) continue;
        int thecount = 0;
        if (codecounts.containsKey(closestCode))
            thecount = codecounts.get(closestCode);
        if (document.hasKey)
            thecount += 2;
        else
            thecount++;
        codecounts.put(closestCode, new Integer(thecount));
        x++;

    }
    String closestCode = getClosestCode(codecounts);
    return closestCode;
}

Please comment with a reason when downvotting. Looks like a perfectly valid question to me. — MK., Sep 05 '11 at 02:28
Why are you testing a `private` class? Why is it `static` if you want to test it? There will be lots of details in the answers and reference material as to why these keywords shouldn't be in methods you are testing. — StuperUser, Sep 05 '11 at 09:47
@MK: It could be considered too localized, although that's not even what the vote is on. Very odd. — Stefan Kendall, Sep 05 '11 at 14:01

score 7 · Answer 1 · answered Sep 05 '11 at 02:16

Well, first of all, I wonder if the method really needs to be static, and what that class is doing. It looks like it might be a GOD class, or at the very least it's violating the single responsibility principle. What does getClosestCode do? If it was a class, you could inject it with a stub in your tests into the test class.

EasyMock will let you mock the method response, but I'm not sure how you mock static methods.

In general, you probably need to

Extract long functions into classes
Make functionality non-static
Maintain the single responsibility principal

score 5 · Accepted Answer · answered Sep 05 '11 at 02:25

It sounds to me like getClosestCode and getClosestDocumentCode belong to a different set of responsibilities than the findPrincipal method. So you'll want to begin by separating these into two different classes. Create an interface for each class to implement. The class that implements the findPrincipal method can then rely on the other interface as a constructor argument, like this:

public class PrincipalFinderImpl implements PrincipalFinder
{
    private CodeFinder codeFinder;
    public PrincipalFinderImpl(CodeFinder codeFinder) {
        this.codeFinder = codeFinder;
    }
    public String findPrincipal(List<DocumentKey> documentkeys_) {
        Hashtable<String, Integer> codecounts = new Hashtable<String, Integer>();
        for (DocumentKey document : documentkeys_) {
            int x = 0;
            String closestCode = codeFinder.getClosestDocumentCode(document.candidates);
            if (closestCode == null) continue;
            int thecount = 0;
            if (codecounts.containsKey(closestCode))
                thecount = codecounts.get(closestCode);
            if (document.hasKey)
                thecount += 2;
            else
                thecount++;
            codecounts.put(closestCode, new Integer(thecount));
            x++;

        }
        String closestCode = codeFinder.getClosestCode(codecounts);
        return closestCode;
    }
}

Now it should be easy to create another class the implements the CodeFinder interface, either manually or using a Mocking framework. You can then control the results of each call to getClosestCode and getClosestDocumentCode, and ensure that each of these methods gets called with exactly the arguments you expect it to be called with.

+1: In my experience, this question *always* indicates that the class should really be two or more classes. — Ryan Stewart, Sep 05 '11 at 02:41
+1 for a code example of what the refactoring might look like. — Stefan Kendall, Sep 06 '11 at 16:52

score 0 · Answer 3 · edited May 23 '17 at 11:56

0

I don't read the method deeply. But if a private method needs to test, it indicates something wrong with your design. At least Kent Beck thinks so.

edited May 23 '17 at 11:56

Community

1
1

answered Sep 05 '11 at 09:44

卢声远 Shengyuan Lu

31,208
22
85
130

"There's something wrong with your design" is extremely unhelpful. – Stefan Kendall Sep 06 '11 at 15:06

score -1 · Answer 4 · answered Sep 05 '11 at 02:11

-1

There is a chapter on stub calls on JUnit Second Edition, i recommend you have a look at that if you think your existing codes are not written to test-driven development standards.

answered Sep 05 '11 at 02:11

Oh Chin Boon

23,028
51
143
215

How to refactor a method to make it easier to test

4 Answers4