HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Grouping rectangles horizontally and vertically

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
rectanglesgroupinghorizontallyverticallyand

Problem

As you can see the below code for each method is that same, except for the properties it uses. For example X vs Y and Width vs Height. Is there anyway that I can refactor the code to use a common main method that changes on which property it uses?

Notes:

The all caps constant is BOX_LINEUP_TOLERANCE has a value of 0.15f and is all caps due to a company policy.

The two methods:

```
private List GroupRectanglesHorizontally(Rectangle[] rectangles, out int averageGap)
{
List nlGaps = new List();
List rectangleGroups = new List();
for (int bi = 0, bc = rectangles.Length; bi g.Contains(rectangles[bi])))
{
continue;
}

int nTolerance = (int)Math.Ceiling(rectangles[bi].Height * BOX_LINEUP_TOLERANCE);
Rectangle[] baSimiliarHeight = rectangles.Where(b => Math.Abs(b.Y - rectangles[bi].Y) rectangles[bi].X)
.ToArray();
if (baSimiliarHeight.Length b.X).ToArray();

int nInitialGap = baSimiliarHeight[0].X - (rectangles[bi].X + rectangles[bi].Width);
int nInitialGapTolerance = (int)Math.Ceiling(nInitialGap * BOX_LINEUP_TOLERANCE);
nlGaps.Add(nInitialGap);

List rectangleList = new List();
rectangleList.Add(rectangles[bi]);
rectangleList.Add(baSimiliarHeight[0]);

for (int sbi = 1; sbi = nInitialGap - nInitialGapTolerance)
{
rectangleList.Add(baSimiliarHeight[sbi]);
nlGaps.Add(nGap);
}
else if (nGap > nInitialGap)
{
break;
}
else
{
//Initial gap was larger so first rectangle is in its own group
rectangleList = new List(new Rectangle[] { rectangles[bi] });
break;
}
}

rectangleGroups.Add(rectangleList.ToArray());
}

averageGap = (int)nlGaps.Average();
return rectangleGroups;
}

pri

Solution

You can have a boolean as a parameter and select X or Y and Width or Height based on that parameter. You can declare the method like this:

private List GroupRectangles(Rectangle[] rectangles, bool horizontally, out int averageGap)


You can use the code from the horizontal method and modify the property calls to use the boolean. For example, you would replace rectangle.X with horizontally ? rectangle.X : rectangle.Y and rectangle.Width with horizontally ? rectangle.Width : rectangle.Height.

It can then be used in your previous methods:

private List GroupRectanglesHorizontally(Rectangle[] rectangles, out int averageGap)
{
    return GroupRectangles(rectangles, true, out averageGap);
}

private List GroupRectanglesVertically(Rectangle[] rectangles, out int averageGap)
{
    return GroupRectangles(rectangles, false, out averageGap);
}


Alternatively, you could have functions as parameters for selecting the properties, like using a lambda expression in OrderBy().

Edit: as for the first method itself:

In the following line of code:

Rectangle[] baSimiliarHeight = rectangles.Where(b => Math.Abs(b.Y - rectangles[bi].Y)  rectangles[bi].X)
                                                .ToArray();


it won't get rectangles that are at the exact same X position. But that won't matter if it's guaranteed that none of the rectangles overlap. Then there's this code right after:

if (baSimiliarHeight.Length <= 1)
{
    rectangleGroups.Add(new Rectangle[] { rectangles[bi] });
    continue;
}


Because rectangles with the same X position weren't matched before, baSimiliarHeight won't contain rectangles[bi], so if it has a length of 1, that 1 element will be a different rectangle, but only the original rectangle will get added as a group.

In the inner for loop, the body of the last else only adds the initial rectangle, but doesn't do anything about the previously added nGaps, so it's possible for the loop to add a bunch of nGaps but only add the initial rectangle.

If you get a list of the rectangles ordered by their X position right away and loop through that instead of the original list, you can take some shortcuts in the body of the for loop.

Edit 2:

You should add more descriptive names. I don't know what bi, bc, the nl in nlGaps, etc. stand for. Short names are okay for lambda expressions and loop variables, but even then you should stick to x, y, i, j, or a letter that relates to the object. I think it would make more sense to use r for a rectangle than b.

Code Snippets

private List<Rectangle[]> GroupRectangles(Rectangle[] rectangles, bool horizontally, out int averageGap)
private List<Rectangle[]> GroupRectanglesHorizontally(Rectangle[] rectangles, out int averageGap)
{
    return GroupRectangles(rectangles, true, out averageGap);
}


private List<Rectangle[]> GroupRectanglesVertically(Rectangle[] rectangles, out int averageGap)
{
    return GroupRectangles(rectangles, false, out averageGap);
}
Rectangle[] baSimiliarHeight = rectangles.Where(b => Math.Abs(b.Y - rectangles[bi].Y) <= nTolerance
                                                        && b.X > rectangles[bi].X)
                                                .ToArray();
if (baSimiliarHeight.Length <= 1)
{
    rectangleGroups.Add(new Rectangle[] { rectangles[bi] });
    continue;
}

Context

StackExchange Code Review Q#132203, answer score: 4

Revisions (0)

No revisions yet.