Refactoring repeated loop code with differing nested functions

Question

I've got two for-loops going through a 2D pixel array for image manipulation purposes. I have various different methods I want to perform that will change what function is carried out on each pixel. However, the code for the for loops is repeated inside every one of these methods.

Is there some way I can pull it out?

/**
 * Get pixel array from current image.
 */
public void updatePixels() {
    pixels = new int[IMAGE_HEIGHT][IMAGE_WIDTH];
    for (int row = 0; row < IMAGE_HEIGHT; row++) {
        for (int col = 0; col < IMAGE_WIDTH; col ++) {
            pixels[row][col] = image.getRGB(col, row);
        }
    }
}

/**
 * Set pixel array to current image.
 */
public void commitPixels() {
    for (int row = 0; row < IMAGE_HEIGHT; row++) {
        for (int col = 0; col < IMAGE_WIDTH; col ++) {
            image.setRGB(col, row, pixels[row][col]);
        }
    }
}

public void greyscale() {
    updatePixels();
    
    for (int row = 0; row < IMAGE_HEIGHT; row++) {
        for (int col = 0; col < IMAGE_WIDTH; col++) {
            Color currentColour = new Color(pixels[row][col]);
            int r = currentColour.getRed();
            int g = currentColour.getGreen();
            int b = currentColour.getBlue();
            
            int avg = Math.round((float)((r + g + b) / 3));
            pixels[row][col] = new Color(avg, avg, avg).getRGB();
        }
    }
    commitPixels();
}

public void mirror() {
    updatePixels();
    
    int [][] pixelCopy = pixels.clone();
    
    for (int row = 0; row < IMAGE_HEIGHT; row++) {
        for (int col = 0; col < IMAGE_WIDTH; col++) {
            pixels[row][col] = pixelCopy[row][IMAGE_WIDTH - col - 1];
        }
    }
    commitPixels();
}

Answer 1

You can hide the loop inside a method that takes a BiConsumer as parameter that gets the current column and row passed:

private void forEachPixel(BiConsumer<Integer, Integer> consumer) {
    for (int row = 0; row < pixels.length; row++) {
        for (int col = 0; col < pixels[0].length; col++) {
            consumer.accept(row, col);
        }
    }
}

Now, you can write lambdas for what should be done inside the nested loop:

public void updatePixels() {
    forEachPixel((row, col) -> pixels[row][col] = image.getRGB(col, row));
}

public void commitPixels() {
    forEachPixel((row, col) -> image.setRGB(col, row, pixels[row][col]));
}

public void greyscale() {
    updatePixels();
    forEachPixel((row, col) -> {
        Color currentColour = new Color(pixels[row][col]);
        int r = currentColour.getRed();
        int g = currentColour.getGreen();
        int b = currentColour.getBlue();
        int avg = Math.round(((r + g + b) / 3f));
        pixels[row][col] = new Color(avg, avg, avg).getRGB();
    });
    commitPixels();
}

public void mirror() {
    updatePixels();
    int[][] pixelCopy = Arrays.stream(pixels).map(int[]::clone).toArray(int[][]::new);
    forEachPixel((row, col) -> pixels[row][col] = pixelCopy[row][pixels[0].length - col - 1]);
    commitPixels();
}

Remark: In your question you are using .clone() on a 2d array inside mirror(). This creates a shallow-clone but you need a deep-clone for the logic that follows. This problem is fixed in my answer. In the greyscale() method, don't cast to float after the division but make the divisor a float (see my answer).

Answer 2

Since the loops are identical you can do all the actions on pxels in one method

public void greyscale() {
    pixels = new int[IMAGE_HEIGHT][IMAGE_WIDTH]; // updatePixels
    for (int row = 0; row < IMAGE_HEIGHT; row++) {
        for (int col = 0; col < IMAGE_WIDTH; col++) {
            pixels[row][col] = image.getRGB(col, row); // updatePixels
            Color currentColour = new Color(pixels[row][col]);
            int r = currentColour.getRed();
            int g = currentColour.getGreen();
            int b = currentColour.getBlue();

            int avg = Math.round((float)((r + g + b) / 3));
            pixels[row][col] = new Color(avg, avg, avg).getRGB();
            image.setRGB(col, row, pixels[row][col]); // commitPixels
        }
    }
}

Answer 3

From the code it seems like the updatePixels and commitPixel could be moved inside the for loop of greyscale

// updatePixel and commitPixel could be made inline as well.
public void updatePixel(int row, int col) {
    pixels[row][col] = image.getRGB(col, row);
}
    

public void commitPixel(int row, int col) {
    image.setRGB(col, row, pixels[row][col]);    
}

public void computeColor(int row, int col) {
    Color currentColour = new Color(pixels[row][col]);
    int r = currentColour.getRed();
    int g = currentColour.getGreen();
    int b = currentColour.getBlue();
                
    int avg = Math.round((float)((r + g + b) / 3));
    pixels[row][col] = new Color(avg, avg, avg).getRGB();
}
    
public void greyscale() {
    pixels = new int[IMAGE_HEIGHT][IMAGE_WIDTH];
    
    for (int row = 0; row < IMAGE_HEIGHT; row++) {
        for (int col = 0; col < IMAGE_WIDTH; col++) {
            updatePixel(row, col);
            computeColor(row, col);
            commitPixel(row, col);
        }
    }       
}

With this approach you can reduce the number of times the for loop is executed from 3 to 1.