Refactoring two almost identical methods

Question

I have two almost identical methods which filter the list and returns the filtered result. Their algorithm is identical, the difference is that the first function returns the most frequent element and the second returns the least frequent:

private static List<List<Character>> filter(List<List<Character>> lines, int charIndex) {
        List<List<Character>> result = copyList(lines);

        List<List<Character>> startWith0 = new ArrayList<>();
        List<List<Character>> startWith1 = new ArrayList<>();

        for(int i = 0; i < result.size(); i++) {
            List<Character> currentLine = result.get(i);
            if (currentLine.get(charIndex) == '1') {
                startWith1.add(currentLine);
            } else if (currentLine.get(charIndex) == '0') {
                startWith0.add(currentLine);
            }
        }

        if (startWith1.size() > startWith0.size() ||
        startWith1.size() == startWith0.size()) {
            return startWith1;
        } else {
            return startWith0;
        }
    }

The end of the second function looks like this:

if (startWith1.size() > startWith0.size() ||
        startWith1.size() == startWith0.size()) {
        return startWith0;
} else {
        return startWith1;
}

I think that this duplication of code is not a good program design but I don't see the good way to divide the first part of the function and second one into the different methods.

rzwitserloot · Accepted Answer · 2021-12-27 14:21:55Z

You have a few options.

Helper method

Make a helper method (it is private, its name starts with the method it is a helper for, though in this case, as its a helper for 2, that might be a little tricky), which does all the work, and has a state parameter that indicates how to do the last part. In this case, your state param can simply be a boolean. For other such situations, often an enum (which you can also write in the same source file, and declare as private) is more appropriate.

This helper would end in something like:

boolean winner = startWith1.size() < startWith0.size();
return most == winner ? startWith1 : startWith0;

And your current filter method becomes a one-liner:

public List<Character> filterMost(List<List<Character>> lines, int charIdx) {
  return filter(lines, charIdx, true);
}

private List<Character> filter(List<List<Character>> lines, int charIdx, boolean most) {
 ...
}

Lambdas

You can pass a function that selects what to do based on an input of the 2 lists. Effectively it's the same as the first answer (still involves helper methods), but instead of writing the different code paths in the helper, you write them in the actual methods (passing them to the helper). It's more complicated but can result in easier to maintain code. Generally, 'shorter, less complex' code always wins from 'longer, more complex, but theoretically more maintainable' code, so in this case I doubt this is the right move. However, in cases where there are more states and the different part is more involved, this can be the better answer. Looks like this:

public List<Character> filterMost(List<List<Character>> lines, int charIdx) {
 ...
}

public List<Character> filterLeast(List<List<Character>> lines, int charIdx) {
  return filter(lines, charIdx, (list0, list1) -> {
    if (list0.size() < list1.size()) return list0;
    return list1;
  };
}

private List<Character> filter(List<List<Character>> lines, int charIdx, BinaryOperator<List<Character>> op) {
 ...

return op.apply(startWith0, startWith1);
}

Stefan D. · Accepted Answer · 2021-12-27 14:30:01Z

IMHO you can factor out the loop which is filling the two ArrayLists and - of course - give both methods selfexplaining names like filerMostFrequent/filterLeastFrequent or so:

    private static List<List<Character>> filerMostFrequent(List<List<Character>> lines, int charIndex) {
        List<List<Character>> result = copyList(lines);

        List<List<Character>> startWith0 = new ArrayList<>();
        List<List<Character>> startWith1 = new ArrayList<>();

        filter(charIndex, result, startWith0, startWith1);

        if (startWith1.size() > startWith0.size() ||
        startWith1.size() == startWith0.size()) {
            return startWith1;
        } else {
            return startWith0;
        }
    }

    private static List<List<Character>> filterLeastFrequent (List<List<Character>> lines, int charIndex) {
        List<List<Character>> result = copyList(lines);

        List<List<Character>> startWith0 = new ArrayList<>();
        List<List<Character>> startWith1 = new ArrayList<>();

        filter(charIndex, result, startWith0, startWith1);

        if (startWith1.size() > startWith0.size() ||
                startWith1.size() == startWith0.size()) {
                return startWith0;
        } else {
                return startWith1;
        }
    }

    private static void filter(int charIndex, List<List<Character>> result, 
            List<List<Character>> startWith0,
            List<List<Character>> startWith1) {
        
        for(int i = 0; i < result.size(); i++) {
            List<Character> currentLine = result.get(i);
            if (currentLine.get(charIndex) == '1') {
                startWith1.add(currentLine);
            } else if (currentLine.get(charIndex) == '0') {
                startWith0.add(currentLine);
            }
        }
    }

Collectives™ on Stack Overflow

Refactoring two almost identical methods

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related