Simplifying a method in java

Question

I'm trying to create a simple method which I have below:

public void analyzeWithAnalytics(String data) {

    for (int i = 0; i < VALUE; i++) {
        if (data.equals("action1")) {
            // call a method on a value
        }
        if (data.equals("action2")) {
            // call a different method on a value
        }
    }

This is only a small snippet (I took a lot out of my code), but essentially I want to be able to call a specific method without testing multiple lines in my for loop for which method to call.

Is there a way for me to decide what value to call by declaring a variable at the very beginning, instead of doing so many 'if statement' tests?

OK, I have an ArrayList inside my class:

private List<Value> values;

The value object has 2 fields time and speed.

Depending on the string I pass (time or speed), I want to be able to call the specific method for that field without doing multiple string comparisons on what method I passed.

For example, I want to be able to call getSpeed() or getTime() without doing a string comparison each time I want to call it.

I just want to test it once.

Are those two if-statements meant to be identical? And do you plan to perform the same method on every iteration of your loop? — PakkuDon, Feb 19 '14 at 22:29
It may be that you have severe design flaws in your program, unfortunately, we can't see them. To be honest, the name of your method seems to indicate great confusion on your side .... — Ingo, Feb 19 '14 at 22:33
Presumably there's other code in-between the two if statements?? There's no really good way to handle this that I've found. You can obviously set bools or an int index to indicate what "action" is to be executed, to avoid repeated `equals` calls. But otherwise you'd have to implement a messy inner class or some such -- worthwhile in some cases but not in the general case. — Hot Licks, Feb 19 '14 at 22:34

score 2 · Accepted Answer · answered Feb 19 '14 at 22:59

2

Another one:

enum Action {
    SPEED {
        public void doSomething() {
            // code
        }
    },
    TIME {
        public void doSomething() {
            // code
        }
    };

    public abstract void doSomething();
}


public void analyzeWithAnalytics(Action data) {
    for (int i = 0; i < VALUE; i++) {
        data.doSomething();
    }
}

answered Feb 19 '14 at 22:59

Axel

13,939
5
50
79

Edit: Actually nevermind my response was stupid. Thanks this solution works. – user1413969 Feb 19 '14 at 23:06

Ashot Karakhanyan · Answer 2 · 2014-02-19T22:50:46.807

1

You can have a Map which maps the names (action1, action2, ...) to classes which common parent and one method. And make call as following:

map.getClass("action1").executeMethod();

Map<String, MethodClass> theMap = new Map<>();

interface MethodClass {
    executeMethod();
}

and children:

class MethodClass1 implements MethodClass{...}
class MethodClass2 implements MethodClass{...}

edited Feb 19 '14 at 22:50

answered Feb 19 '14 at 22:42

Ashot Karakhanyan

2,804
3
23
28

1

better use an enum with an overridden `executeMethod` for each enum value. It saves you the map. – Axel Feb 19 '14 at 22:54
Yes, good approach. I think in `Java 8` it will look mach better with `lambda`. – Ashot Karakhanyan Feb 19 '14 at 22:57

John Dorian · Answer 3 · 2014-02-20T00:31:40.130

0

You could try something like this, it would reduce the amount of typing for sure:

public void analyzeWithAnalytics(String data) {

    for (int i = 0; i < VALUE; i++) {
        switch(data) {
            case "action1": doSomething(); break;
            case "action2": doSomething(); break;
        }
    }
}

edited Feb 20 '14 at 00:31

answered Feb 19 '14 at 22:34

John Dorian

1,884
1
19
29

3

Note, that `switch` on `String` is only possible from Java SE 7 upwards. And your syntax is wrong (the value at the `case` must be in quotes). – qqilihq Feb 19 '14 at 22:39

score 0 · Answer 4 · answered Feb 19 '14 at 22:47

Your goal is not really clear from your question. Do you want to:

avoid typing the many cases?
gain code readability?
improve performance?

In case you're after performance, don't optimize prematurely! Meaning, don't assume that this will be important for performance without checking that out first (preferably by profiling). Instead focus on readability and perhaps laziness. ;)

Anyway, you can avoid the many tests inside by simply checking data outside of the loop. But than you'd have to copy/paste the loop code several times. Doesn't make the method more beautiful...

I would also recommend using case instead of if. It improves readability a lot and also gives you a little performance. Especially since your original code didn't use if - elseif - ... which means all conditions are checked even after the first was true.

My main goal would be to have code readability. Ideally I want to declare a method variable in the very beginning of my method based on what string I passed (like in functional programming). And each time I run into a specific piece of code that I want to execute, I execute the method that I declared in the beginning of my method. — user1413969, Feb 19 '14 at 22:57

score 0 · Answer 5 · answered Feb 19 '14 at 22:51

0

Do I get this right? data will not be changed in the loop? Then do this:

public void analyzeWithAnalytics(String data) {

    if (data.equals("action1")) {
        for (int i = 0; i < VALUE; i++) {
            // call a method on a value
        }
    } else if (data.equals("action2")) {
        for (int i = 0; i < VALUE; i++) {
            // call a different method on a value
        }
    }
}

You can also switch on strings (Java 7) if you don't like ìf...

answered Feb 19 '14 at 22:51

Axel

13,939
5
50
79

Yes, this is a good comment. But the main thing is, the code inside my loops are very long. I didn't want to repeat the same code (barring the method name changes) twice since that looks ugly. – user1413969 Feb 19 '14 at 22:55

Simplifying a method in java

5 Answers5