Logic inside loops? Or rather two separate almost identical loops?

Question

NOTE: I'm under the impression that you want to avoid logic statements inside loops. This I believe was stated partly because of how the compiler can optimize any loop where iterations behave predictable. While I'm almost certain I've heard this before and for a long time I've thought of this as a convention. Sadly I couldn't find any good references. If this is true however there are some conflicting cases due to the "DRY" principle (Don't repeat yourself).

PRETEXT: Assuming that you have a rather sizable data set, say a multi-dimensional array as I used in this example. Furthermore assume you need to traverse every entry and do some operation on all or some of the elements, and that you need to be able to choose that one or another operation is to be performed. This calls for making two methods where 90%-99% of the code is identical between the two, while only an operator or a method call is different. If this had been C++ I would have wanted to provide a function pointer to a loop function, though I do not know if this too would be preferred avoided.

QUESTION: Would it be preferred to use logic statements and have only one loop or rather two almost identical methods?

EXAMPLE: I've provided some example to show how redundant the "twin" method solution looks:

// This method is provided for completeness of the example
// and to provide some clue as to what boolean parameter and logic statement 
// I could alternatively have implemented within the loop method instead of external to it.
public int[][] addOrSubtractArrays(int[][] a, int[][] b, boolean useAdd){
    if(a == null || b == null || a.length != b.length || a.length < 1 || a[0].length != b.length)
        return null;
    return useAdd ? add(a, b) : subtract(a, b);
}

private int[][] add(int[][] a, int[][] b){
    int h = a.length;
    int w = a[0].length;
    int[][] c = new int[h][w];
    for(int y = 0; y < h; y++){
        for(int x = 0; x < w; x++){
            c[y][x] = a[y][x] + b[y][x];
        }
    }
    return c;
}

private int[][] subtract(int[][] a, int[][] b){
    int h = a.length;
    int w = a[0].length;
    int[][] c = new int[h][w];
    for(int y = 0; y < h; y++){
        for(int x = 0; x < w; x++){
            c[y][x] = a[y][x] - b[y][x];
        }
    }
    return c;
}

EXAMPLE 2: The (obvious?) alternative

private int[][] addOrSubtract(int[][] a, int[][] b, boolean useAdd){
    if(a == null || b == null || a.length != b.length || a.length < 1 || a[0].length != b.length)
        return null;
    int h = a.length;
    int w = a[0].length;
    int[][] c = new int[h][w];
    for(int y = 0; y < h; y++){
        for(int x = 0; x < w; x++){
            if(useAdd)
                c[y][x] = a[y][x] + b[y][x];
            else
                c[y][x] = a[y][x] - b[y][x];
        }
    }
    return c;
}

I am overly tempted to make some common method holding the entire loop structure to avoid (almost) duplicate code. However if "what I've heard" has some reasonable context, this may to my knowledge be best avoided.

Answer 1

As you have said, if this were C++, you would pass a function pointer. Well, there's something similar if you're using Java 8 - BiFunction<Integer, Integer, Integer>

Your single method will look like this:

private int[][] addOrSubtract(int[][] a, int[][] b, boolean useAdd){
    BiFunction<Integer, Integer, Integer> func = useAdd ? 
        ((a, b) -> a + b) : ((a, b) -> a - b);
    if(a == null || b == null || a.length != b.length || a.length < 1 || a[0].length != b.length)
        return null;
    int h = a.length;
    int w = a[0].length;
    int[][] c = new int[h][w];
    for(int y = 0; y < h; y++){
        for(int x = 0; x < w; x++){
            c[y][x] = func.apply(a[y][x], b[y][x]);
        }
    }
    return c;
}

If you're not using Java 8, then I guess you can just use either of your two solutions. I don't think there's anything wrong with them. And remember, premature optimization is the root of all evil!

Answer 2

this could be out of your question's scope, as i don't have an answer if logic in loop is good or not, but it's always good to consider other designs. like this one, it does not have a repeated code, and it does not have a logic in the for loop

public int[][] addOrSubtractArrays(int[][] a, int[][] b, boolean useAdd){
    if(a == null || b == null || a.length != b.length || a.length < 1 || a[0].length != b.length)
        return null;
    int h = a.length;
    int w = a[0].length;
    int[][] c = new int[h][w];
    for(int y = 0; y < h; y++){
        for(int x = 0; x < w; x++){
            c[y][x] = getResult(a[y][x] , b[y][x], useAdd);
        }
    }

    return c;
}

private int getResult(int elementA, int elementB, boolean useAdd){
    return useAdd ? (elementA+elementB) : (elementA-elementB);
}

Answer 3

addOrSubtract is bad. What about other forms of arithmetic, such as multiply? You could expose a multiplyOrDivide, but what if a time comes where the choices should be addOrMultiply? Doesn't scale well.

What you should have is a method that allows the client to specify what operation to perform:

public int[][] calculate(int[][] first, int[][] second, Operation operation) {
    if(firstArray == null || secondArray == null || firstArray.length != secondArray.length || firstArray.length < 1 || firstArray[0].length != secondArray.length)
        throw new IllegalArgumentException("Arrays can't be null and must be of equal length.");

    int height = firstArray.length;
    int width = firstArray[0].length;
    int[][] result = new int[height][width];
    for(int y = 0; y < height; y++){
        for(int x = 0; x < width; x++){
            result[y][x] = operation.performOn(firstArray[y][x], secondArray[y][x]);
        }
    }
}

You can now add new operations as needed:

enum Operation {
    ADD {
        @Override
        public void performOn(int firstValue, int secondValue) {
            return firstValue + secondValue;
        }
    },
    SUBTRACT {
        //...
    };

    public abstract int performOn(int firstValue, int secondValue);
}

If you feel overriding in this fashion makes scaling too verbose, you can make use of the strategy pattern by delegating the logic to a callback function:

enum Operation { //could/should implement IOperation
    ADD((a, b) -> a + b),
    SUBTRACT((a, b) -> a - b);

    private IOperation operation;

    Operation(IOperation operation) {
        this.operation = operation;
    }

    public final int performOn(int firstValue, int secondValue) {
        return operation.performOn(firstValue, secondValue);
    }
}

interface IOperation {
    int performOn(int firstValue, int secondValue);
}

A client can now use your function as demonstrated below:

calculate(firstArray, secondArray, Operation.ADD);

The reason I chose to create a new functional interface rather than use BiFunction is to avoid autoboxing. Performance seems to be a worry of yours, and autoboxing can dramatically affect performance, especially if you'll be performing this intensely. Whether it be from arrays having large sizes, or from being required to call addOrSubtract continuously within a small timeframe, it's best to avoid the pitfall.

The IllegalArgumentException allows the program to "blow up" with a descriptive message. You returned null, which means whoever uses this method needs to perform a null-check (clutter-some, code smell, the billion dollar mistake) or they might encounter a NullPointerException which does not have a descriptive message.