Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

You should avoid using static variablesstatic 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.

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.

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.

Source Link
mariosangiorgio
  • 2.6k
  • 1
  • 15
  • 19

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.