Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Doing homework, implementation of the algorithm flood fills. I'm writing a program to this guide: http://en.wikipedia.org/wiki/Flood_fill. And I have some questions:

  1. Is it normal to specify the parameters in function replaces the color of any character bucketFill.fill (0, 0, '*', 'O');, I do not know what kind of color will be initially to these coordinates?
  2. An algorithm is correct? I wrote it for example in Wikipedia, but the result of my program is as follows:

    @@@@@
    @@@@@
    @@@@@
    @@@@@
    @@@@@
    @@@@@
    @@@@@
    

    I think must be this:

    *@@@@
    @@@@@
    @@#@@
    @@@@@
    @@@@@
    @@@##
    @@@@@
    

My code:

class BucketFill {

    private char[][] pixels;

    public BucketFill(char[][] pixels) {
        this.pixels = pixels;
    }

    public void fill(int x, int y, char newColor, char oldColor) {
        if (x < 0) return;
        if (y < 0) return;
        if (x >= pixels.length) return;
        if (y >= pixels[x].length) return;

        oldColor = pixels[x][y];

        if (newColor == pixels[x][y]) return;
        if (oldColor != pixels[x][y]) return;

        pixels[x][y] = newColor;

        fill(x - 1, y, newColor, oldColor);
        fill(x + 1, y, newColor, oldColor);
        fill(x, y - 1, newColor, oldColor);
        fill(x, y + 1, newColor, oldColor);
    }

    public void inspect() {
        for (int y = 0; y < pixels.length; y++) {
            for (int x = 0; x < pixels[y].length; x++) {
                System.out.print(pixels[y][x]);
            }
            System.out.print("\n");
        }
    }

    public static void main(String argv[]) {
        char pixels[][] =
        {
            { 'O', 'X', 'X', 'X', 'X' },
            { 'X', 'O', 'O', 'O', 'X' },
            { 'X', 'O', '#', 'O', 'X' },
            { 'X', 'O', 'O', 'O', 'X' },
            { 'X', 'X', 'X', 'X', 'X' },
            { 'X', 'X', 'X', '#', '#' },
            { 'X', 'X', 'X', 'X', 'X' }
        };
        BucketFill bucketFill = new BucketFill(pixels);
        bucketFill.fill(0, 0, '*', 'O');
        bucketFill.fill(3, 0, 'O', 'O');
        bucketFill.fill(2, 1, '@', 'O');
        bucketFill.inspect();
    }
}
share|improve this question

1 Answer 1

up vote 4 down vote accepted
  1. Normally there's no need to specify the color to change from. You only need to specify the coordinates and leave it up to the flood fill routine to find out what color is at that location. I would define another method that is public, and make the recursive method a private implementation method. The public method would look up the color at the location and then call the private method.

    public void fill(int x, int y, char newColor) {
        fill(x, y, newColor, pixels[x][y]);
    }
    
    private void fill(int x, int y, char newColor, char oldColor) {
        ...
    }
    
  2. You are not using oldColor correctly; you're overwriting the parameter value. The line:

    oldColor = pixels[x][y];
    

    should either be deleted or (better) amended to:

    char color = pixels[x][y];
    

    And the lines:

    if (newColor == pixels[x][y]) return;
    if (oldColor != pixels[x][y]) return;
    

    should then be changed to use the color variable:

    if (newColor == color) return;
    if (oldColor != color) return;
    

    Other than that, you have implemented the algorithm correctly.

    It's worth noting that the way this algorithm uses recursion could lead to a stack overflow for a large block of color. Also, by dint of the +1 and -1, will end up calling fill() multiple times for each cell.

share|improve this answer
    
Hi! Thank you for answer. I updated code use your tips, please, check. -) – rel1x Dec 8 '14 at 15:18
    
It looks good. There are style issues, e.g. current_color would be more consistently named as currentColor. I would encourage you to always use braces { } after conditionals. You might consider combining some of the conditionals and creating some helper methods to make things easier to read, e.g. char colorAt(x, y). setColor(int x, int y, char color). You might also consider upvoting this answer as I've spent some significant time on it. – aetheria Dec 8 '14 at 16:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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