5
\$\begingroup\$

Implement the "paint fill" function of image editing programs: Given a screen point and new color fill the surrounding area.

Any comments on the code below?

Time complexity: \$\mathcal{O}(N^2)\$

Space complexity: \$\mathcal{O}(N^2)\$

  static int N = 6;
  static boolean[][] colored = new boolean[N+2][N+2];
  static int old_color;

  public static void fillP(int[][] array, int x, int y, int color){
    System.out.println("Before");
    printArray2D(array);
    old_color = array[x][y];
    fillPaint(array, x, y, color);
    System.out.println("After");
    printArray2D(array);
  }

  public static void fillPaint(int[][] array, int x, int y, int color){
    if (x < 0 || x > N || y < 0 || y > N) return;
    if (array[x][y] != old_color) return;

    array[x][y] = color;
    colored[x+1][y+1] = true;

    for (int i = -1; i < 2; i += 2){
      for (int j = -1; j < 2; j += 2){
        if (!colored[x+i+1][y+1]){
          fillPaint(array, x+i, y, color);
        }
        if (!colored[x+1][y+j+1]){
          fillPaint(array, x, y+j, color);
        }
      }
    }
  }


  public static void printArray2D(int[][] array){
    int n = array.length;
    int m = array[0].length;

    for (int i = 0; i < n; i++){
      for (int j = 0; j < m; j++){
        if (j == m-1){
          System.out.print(array[i][j] + " ");
          System.out.println();
        }
        else{
          System.out.print(array[i][j] + " ");
        }
      }
    }
  }
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

You should avoid using static variables. They're going to cause you troubles and make your code untestable and very difficult to maintain.

The names of your variables are a bit confusing. What does colored mean? Why do you need to keep track of old_color? At a first glance at the code I cannot tell what they are for.

Why is fillP doing two different things? I would completely get rid of all the printing it does, I don't think it is really useful. Actually I'd get rid of it entirely and use only fillPaint instead.

I'd try to introduce some different data types to better model the problem your're working on. What about using a Canvas class instead of int[][], a Coordinate class instead of a pair of int and a Color class instead of an int? That could help making your program much better structured.

Going back to your static variables, I thing you should get rid of them and instead have two separate functions. One to determine what pixels you need to color and another to actually change their colors. Something like:

public void fillPaint(int[][] canvas, int x, int y, int color)
{
    for(int i = -1; i < 2; i += 2)
    {
        for(int j = -1; j < 2; j += 2)
        {
            setColor(canvas, x + i, y + j, color);
        }
    }
}

I'm not sure if you want to use colored to obtain a particular shape, but you can surely do it within fillPaint adding some conditions before the execution of setColor and you surely don't need to use a global variable. Deciding the shape of the area you want to color is exactly the responsibility of fillPaint.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.