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. — Priyanka W
– Priyanka W, Commented Nov 28, 2019 at 13:50
@PriyankaW That isnt really helpful ... there are better ways to solve this, see the answers. — GhostCat
– GhostCat, Commented Nov 28, 2019 at 13:52

luk2302 · Accepted Answer · 2019-11-28 14:11:26Z

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, 2019 at 14:11

answered Nov 28, 2019 at 13:50

luk2302

57.5k24 gold badges108 silver badges151 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

GhostCat Over a year ago

You came in second, but you had the method reference ;-)

Phil Over a year ago

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.

luk2302 Over a year ago

@Phil then you need to create a new interface of your own, called e.g. ThrowingConsumer according to stackoverflow.com/questions/18198176/…

GhostCat · Accepted Answer · 2019-11-28 14:05:42Z

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, 2019 at 14:05

answered Nov 28, 2019 at 13:50

GhostCat

141k28 gold badges190 silver badges262 bronze badges

1 Comment

Phil Over a year ago

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.

D. Lawrence · Accepted Answer · 2019-11-28 14:01:11Z

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, 2019 at 14:01

answered Nov 28, 2019 at 13:53

D. Lawrence

9591 gold badge10 silver badges23 bronze badges

2 Comments

Phil Over a year ago

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.

D. Lawrence Over a year ago

Unfortunately. you reached usages' limits with checked exception and consumers

Collectives™ on Stack Overflow

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

3 Answers 3

3 Comments

1 Comment

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

1 Comment

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related