How can I factorise this code?

Question

I'm working on a java webserver which calls a web service. I'm trying to write some code in a nicer.

I've got 2 similar functions, only one thing changes : the function i have to call depending what i want to do.

public Object updateDef() {
    ExecutorService pool = Executors.newFixedThreadPool(20);
    List<Callable<String>> tasks = new ArrayList<Callable<String>>();

    logger.info("Updating surv def...");

    for (final Surv surv : Client.activatedSurvs) {
        final Client client = new Client(surv.getName(), surv.getPassword());

        tasks.add(new Callable<String>() {
            public String call() throws Exception {
                HERE -> client.updateDef(surv);
                return surv.getId();
            }
        });
        client.destroy();
    }

    List<Future<String>> results = null;
    try {
        results = pool.invokeAll(tasks);
        for (Future<String> future : results) {
            future.get();

        }
    } catch (ExecutionException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    pool.shutdown();

    return null;
}

And the other one :

public Object updateStats() {
    ExecutorService pool = Executors.newFixedThreadPool(20);
    List<Callable<String>> tasks = new ArrayList<Callable<String>>();

    logger.info("Updating surv Stats...");

    for (final Surv surv : Client.activatedSurvs) {
        final Client client = new Client(surv.getName(), surv.getPassword());

        tasks.add(new Callable<String>() {
            public String call() throws Exception {
                HERE -> client.updateStats(surv).toString();
                return surv.getId();
            }
        });
        client.destroy();
    }

    List<Future<String>> results = null;
    try {
        results = pool.invokeAll(tasks);
        for (Future<String> future : results) {
            future.get();
        }
    } catch (ExecutionException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    pool.shutdown();

    return null;
}

So the 2 functions are updateStats and updateDef from class A with a static list from B. The problem is that I've to pass a parameter which is an existing element from this list.

=> A.updateStats(B.list.get(X));

=> A.updateDef(B.list.get(X));

I visited this link How to pass a function as a parameter in Java? but without success

Any idea ?

this question would be better suited on http://codereview.stackexchange.com/ — Sionnach733, Apr 29 '14 at 14:24
Pass a boolean in the parameters of the function, something like 'isDef' and use with an if statement to call either one of your update lines — Lex Webb, Apr 29 '14 at 14:25
The question you link gives a perfectly valid answer. Any form of `Interface` would work, too. — njzk2, Apr 29 '14 at 14:26
I forgot to say that it has to be the quickest way possible. I'm worrying if statement will slow down my app ? — MacBidouille, Apr 29 '14 at 14:31

score 0 · Answer 1 · answered Apr 29 '14 at 14:24

0

You could use a Strategy pattern or Functors. Honestly, I think I would just factor the common pieces out into shared methods.

answered Apr 29 '14 at 14:24

jgitter

3,396
1
19
26

score 0 · Answer 2 · answered Apr 29 '14 at 14:28

0

Easy - pass in the Callable and don't repeat all the boilerplate around it.

public Object foo(Callable<String> callable) {
    ExecutorService pool = Executors.newFixedThreadPool(20);
    List<Callable<String>> tasks = new ArrayList<Callable<String>>();

    logger.info("Updating surv def...");

    for (final Surv surv : Client.activatedSurvs) {
        final Client client = new Client(surv.getName(), surv.getPassword());
        tasks.add(callable;
        client.destroy();
    }

    List<Future<String>> results = null;
    try {
        results = pool.invokeAll(tasks);
        for (Future<String> future : results) {
            future.get();

        }
    } catch (ExecutionException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    pool.shutdown();

    return null;
}

answered Apr 29 '14 at 14:28

duffymo

305,152
44
369
561

yes, but no, because the Callable depends on the `client` and on the `surv`. – njzk2 Apr 29 '14 at 14:29
My point is pass in the stuff that changes. The idea still holds. – duffymo Apr 29 '14 at 14:32
not really, because the itself callable changes (to make it not change, it should not be just a callable, but also be able to have parameters for client and surv). – njzk2 Apr 29 '14 at 14:34
Give the Callable what it needs as parameters. Can still do it my way. – duffymo Apr 29 '14 at 14:39
A `Callable` does not know any parameters by itself. – njzk2 Apr 29 '14 at 14:41
Only if you write it the way you did. If you create an UpdateCallable that extends the Callable interface and has a ctor that takes parameters you're all set. You're doing it the way most folks learn how to do event listeners for Swing, but it's not the only way. Need not be an anonymous implementation. – duffymo Apr 29 '14 at 14:44
yes, my point exactly. (`it should not be **just** a callable, but also be able to have parameters for client and surv`) – njzk2 Apr 29 '14 at 14:45
anyway there are 2 implementation of whatever class/interface performs/proxies the task. In your case, you'd need 2 implementation too, and I don't think that wether these are anonymous or not is relevant. – njzk2 Apr 29 '14 at 14:48

score 0 · Accepted Answer · answered Apr 29 '14 at 14:40

Your object is to replace:

tasks.add(new Callable<String>() {
        public String call() throws Exception {
            client.updateStats(surv).toString();
            return surv.getId();
        }
    });

With

tasks.add(new Callable<String>() {
        public String call() throws Exception {
            doSomething(client, surv);
            return surv.getId();
        }
    });

Which can be done by defining an Interface:

public interface ClientUpdate {
    public void performUpdate(Client client, Surv surv);
}

Then, use this interface as a parameter to your method:

public Object updateStats() {

Becomes

public Object update(ClientUpdate updater) {

Then use it in your callable

tasks.add(new Callable<String>() {
        public String call() throws Exception {
            updater.performUpdate(client, surv);
            return surv.getId();
        }
    });

Then implement it:

updateStats();

becomes:

update(new ClientUpdate() {
    public void performUpdate(Client client, Surv surv) {
        client.updateStats(surv).toString();
    }
});

Similarly for updateDef();

How can I factorise this code?

3 Answers3