How to refactor methods that only differ in which method they call on an object in java?

Question

Probably kind of a beginner question but I am stuck in my box.

Assuming the following interface:

public interface Foo {
    void one() throws Exception;
    void two() throws Exception;
}

And this class:

class MyClass {
    private Collection<Foo> foos;

    MyClass(Collection<Foo> foos) {
        this.foos = foos;
    }

    public void oneOnAllFoos() {
        // assuming more code...
        for (Foo foo : foos) {
            // assuming more code...
            foo.one(); // the only different line
        }
    }

    public void twoOnAllFoos() {
        // assuming more code...
        for (Foo foo : foos) {
            // assuming more code...
            foo.two(); // the only different line
        }
    }
}

Now in case the oneOnAllFoos and twoOnAllFoos are the same except for the foo one() and two() calls, how can I refactor MyClass to get one method containing all logic letting me specify which method on the Foo objects to be called? I know it is possible using reflection but I think there must be a KISS way, too. Thanks!

Edit: added throws Exception to the interface methods.

Edit2: the // assuming more code... contains the exception handling of the interface method calls. There I collect the thrown exceptions to then throw them further as composite exception (must process all Foos first.

I think you can use if-else inside for loop and call one(), two() accordingly. — Haripriya, Nov 28 '19 at 13:50
@PriyankaW That isnt really helpful ... there are better ways to solve this, see the answers. — GhostCat, Nov 28 '19 at 13:52

luk2302 · Answer 1 · 2019-11-28T14:11:26.980

2

You need to pass in a ThrowingConsumer<Foo>:

interface ThrowingConsumer<T> {
    void accept(T t) throws Exception; // signature very similar to a normal Consumer
}

public void onAllFoos(ThrowingConsumer<Foo> consumer) {
    // assuming more code...
    for (Foo foo : foos) {
        // assuming more code...
        consumer.accept(foo); // handle exception here.
    }
}

Callable via

onAllFoos(Foo::one);

edited Nov 28 '19 at 14:11

answered Nov 28 '19 at 13:50

luk2302

55,258
23
97
137

You came in second, but you had the method reference ;-) – GhostCat Nov 28 '19 at 13:51
Thanks. I tried using a `Consumer` but actually forgot to add one important part which is that the interface methods can throw exceptions (see edit). This results in an error stating that there is an unhandled exception. – Phil Nov 28 '19 at 13:59
1

@Phil then you need to create a new interface of your own, called e.g. `ThrowingConsumer` according to https://stackoverflow.com/questions/18198176/java-8-lambda-function-that-throws-exception – luk2302 Nov 28 '19 at 14:07

GhostCat · Accepted Answer · 2019-11-28T14:05:42.450

1

You can use the Consumer interface here:

private forEachFoo(Consumer<Foo> consumer) {
  for each foo: consumer.accept(foo) ...

to then pass in different consumers with simple lambdas, like:

public void oneOnAllFoos() {
  forEachFoo(f -> f.one());

or, as suggested in the other answer, by using a method reference Foo::one.

Edit: when your methods throw checked exceptions, you can do two use your own Consumer/Function interface see here for details.

edited Nov 28 '19 at 14:05

answered Nov 28 '19 at 13:50

GhostCat

137,827
25
176
248

Thanks. I tried using a `Consumer` but actually forgot to add one important part which is that the interface methods can throw exceptions (see edit). This results in an error stating that there is an unhandled exception. – Phil Nov 28 '19 at 13:59

D. Lawrence · Answer 3 · 2019-11-28T14:01:11.937

0

I feel that the best way to deal with your problem (in Java 8 at least) is to create a private method that takes a Consumer<Foo> as a parameter, such as:

class MyClass {
    private Collection<Foo> foos;

    MyClass(Collection<Foo> foos) {
        this.foos = foos;
    }

    public void oneOnAllFoos() {
        abstractOnAllFoos(Foo::one);
    }

    public void twoOnAllFoos() {
        abstractOnAllFoos(Foo::two);
    }

    private void abstractOnAllFoos(Consumer<Foo> fooConsumer) {
        // assuming more code...
        for (Foo foo : foos) {
            // assuming more code...
            fooConsumer.accept(foo);
        }
    }
}

The choice of using a consumer has been made only because your methods one() and two() aren't returning anything.

edited Nov 28 '19 at 14:01

answered Nov 28 '19 at 13:53

D. Lawrence

943
1
10
23

Thanks. I tried using a `Consumer` but actually forgot to add one important part which is that the interface methods can throw exceptions (see edit). This results in an error stating that there is an unhandled exception. – Phil Nov 28 '19 at 13:58
Unfortunately. you reached usages' limits with checked exception and consumers – D. Lawrence Nov 28 '19 at 14:00

How to refactor methods that only differ in which method they call on an object in java?

3 Answers3