# Thread: How can I make this code more elegant?

1. ## How can I make this code more elegant?

Hello,

I am doing some computer vision work, which involves finding edges in an image, and then tracing round these edges to find solid bodies in an image. After applying my edge detection functions, I then need to find pixels in the image which correspond to an edge, and then essentailly "join the dots" with other pixels which correspond to an edge - until a full loop of edge pixels is found, which will reveal an object.

When I have one pixel that is an edge, I need to test the surrounding 8 pixels to see if they too are an edge. For a number of reasons, it is important to test these pixels in a certain order, based upon the likelihood of each pixel being the next edge of the current contour.

To do this, I find the different between the coordinates of the last two edge pixels, and assume that the contour will similarly follow this direction for the next edge pixel. For example, if the first two edge pixels in the contour are pixelA and pixelB, in that order, and pixelA is directly below pixel B, then it is likely that the third pixel in the contour will again be directly below pixelB.

If there is not an edge pixel found at that location, then the next most likely location is tested - which in this example would be diagonally down and right, or diagonally down and left. The program therefore tests the surrounding 8-pixels in order of how likely they are to contain the next edge pixel - which is based upon the different in coordinates between the last two edge pixels.

Here is some of my program :

Code:
```void main()
{
int i = GetCurrentEdgePoint().x; // sets i to the x-coordinate of the current edge pixel
int j = GetCurrentEdgePoint().y; // sets j to the y-coordinate of the current edge pixel

int i_prev = GetPreviousEdgePoint().x; // sets i_prev to the x-coordinate of the last edge pixel
int j_prev = GetPreviousEdgePoint().y; // sets j_prev to the y-coordinate of the last edge pixel

int i_diff = i - i_prev; // i_diff is the difference between the x-coordinates of the current, and the previous, edge pixels
int j_diff = j - j_prev; // j_diff is the difference between the y-coordinates of the current, and the previous, edge pixels

for (int trial_number = 0; trial_number < 8; trial_number ++)
{
// CvPoint is a structure containing the x and y coordinates of a point.
// Here we set next_diff to the value of the next pixel to try, relative to the current pixel
CvPoint next_diff = GetNextTrial(i_diff, j_diff, trial_number);

// We then add next_diff to the x and y coordinates of the current pixel
int x = i + next_diff.x;
int y = j + next_diff.y;

// Then we test to see whether, at the coordinates (x,y), there lies an edge in the image
if (is_edge(x, y) == true) // is_edge returns true if the pixel at (x, y) is an edge
{
return 1;
}
}
}

// This function takes the difference in coordinates of the last two edge point, and returns the relative location of the next pixel to be tested.
CvPoint GetNextTrial(int i_diff, int j_diff, int trial_number)
{
switch (i_diff)
{
case -1:
switch (j_diff)
{
case -1:
switch (trial_number)
{
case 0:
return cvPoint(-1, -1);
case 1:
return cvPoint(0, -1);
case 2:
return cvPoint(-1, 0);
case 3:
return cvPoint(1, -1);
case 4:
return cvPoint(-1, 1);
case 5:
return cvPoint(1, 0);
case 6:
return cvPoint(0, 1);
}

case 0:
switch (trial_number)
{
case 0:
return cvPoint(-1, 0);
case 1:
return cvPoint(-1, -1);
case 2:
return cvPoint(-1, 1);
case 3:
return cvPoint(0, -1);
case 4:
return cvPoint(0, 1);
case 5:
return cvPoint(1, -1);
case 6:
return cvPoint(1, 1);
}

case 1:
switch (trial_number)
{
case 0:
return cvPoint(-1, 1);
case 1:
return cvPoint(-1, 0);
case 2:
return cvPoint(0, 1);
case 3:
return cvPoint(-1, -1);
case 4:
return cvPoint(1, 1);
case 5:
return cvPoint(0, -1);
case 6:
return cvPoint(1, 0);
}
}

case 0:
switch (j_diff)
{
case -1:
switch (trial_number)
{
case 0:
return cvPoint(0, -1);
case 1:
return cvPoint(-1, -1);
case 2:
return cvPoint(1, -1);
case 3:
return cvPoint(-1, 0);
case 4:
return cvPoint(1, 0);
case 5:
return cvPoint(-1, 1);
case 6:
return cvPoint(1, 1);
}

case 0:
switch (trial_number)
{
case 0:
return cvPoint(1, 0);
case 1:
return cvPoint(1, 1);
case 2:
return cvPoint(1, -1);
case 3:
return cvPoint(0, 1);
case 4:
return cvPoint(0, -1);
case 5:
return cvPoint(-1, 1);
case 6:
return cvPoint(-1, -1);
case 7:
return cvPoint(-1, 0);
}

case 1:
switch (trial_number)
{
case 0:
return cvPoint(0, 1);
case 1:
return cvPoint(-1, 1);
case 2:
return cvPoint(1, 1);
case 3:
return cvPoint(-1, 0);
case 4:
return cvPoint(1, 0);
case 5:
return cvPoint(-1, -1);
case 6:
return cvPoint(1, -1);
}
}

case 1:
switch (j_diff)
{
case -1:
switch (trial_number)
{
case 0:
return cvPoint(1, -1);
case 1:
return cvPoint(0, -1);
case 2:
return cvPoint(1, 0);
case 3:
return cvPoint(-1, -1);
case 4:
return cvPoint(1, 1);
case 5:
return cvPoint(-1, 0);
case 6:
return cvPoint(0, 1);
}

case 0:
switch (trial_number)
{
case 0:
return cvPoint(1, 0);
case 1:
return cvPoint(1, -1);
case 2:
return cvPoint(1, 1);
case 3:
return cvPoint(0, -1);
case 4:
return cvPoint(0, 1);
case 5:
return cvPoint(-1, -1);
case 6:
return cvPoint(-1, 1);
}

case 1:
switch (trial_number)
{
case 0:
return cvPoint(1, 1);
case 1:
return cvPoint(1, 0);
case 2:
return cvPoint(0, 1);
case 3:
return cvPoint(1, -1);
case 4:
return cvPoint(-1, 1);
case 5:
return cvPoint(0, -1);
case 6:
return cvPoint(-1, 0);
}
}
default:
return cvPoint(0, 0);
}
}```
Now, this works absolutely fine. However, it seems a very code-exhaustive way of going about it. Particularly seeing as I now want to test not just the 8 pixels surrounding the current pixel, but the 24 pixels surronding it. Thus, my GetNextTrial() function will be 4 times longer again.

It seems really unneccessary to have to specify for every single case which pixel should next be tested. I'm trying to figure out how I can come up with a generic function GetNextTrial() which returns the next pixel to be tested, without having to literally search for the correct one in its long list in the switch / case section.

Does anybody have any ideas?

Thank you so much!!

2. Do you want to try all 8 directions that you can move to from a given point?

I would use something like:

Code:
```cvPoint dirs[8] = { cvPoint(-1, -1), cvPoint(-1, 0), ..., cvPoint(1, 1) };

for (i = 0; i != 8; ++i) {
if (current_position + dirs[i] is an edge) {
do something
break
}
}```

3. Hi,

Thanks for the help. I don't need to try all 8 directions - I only need to try the directions until the corresponding pixel is an edge pixel. It is the order of trying out these 8 pixels that matters. The solution you suggested would work if this order didn't matter - but I want to try the most likely pixel first (based upon the difference between the previous two pixels), and if that pixel is not an edge, then try the second most likely pixel.

The reason why I want to do this is too speed up my program - testing each of the 8 pixels is unnecessary, considering that most of the time (ie. when the pixels lie along a straight line) the most likely pixel will in fact be an edge pixel.

4. Since you've already run the edge detector, presumably testing means looking at the value of an int or a bool so a simple clockwise loop with a break to terminate it when an edge pixel is found should cost on average 4 comparisons?

It seems that to save time, you are doing two subtractions and then up to 9 comparisons (say, 4.5 on average) or a total of 6.5 operations to decide on which direction to test first, and if the guess is wrong you still have loop around. And now you are planning to increase the complexity of that process still further (maybe triple -- I'm not sure).

Does this really make sense?