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

Interface for obtaining the bounding box for a collection of elements

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

Problem

How could I improve this code?
I used a position interface to avoid code duplication, can it be done better?
Can I reduce the amount of code somehow?

```
interface PositionInterface
{
double getPosition(PageAreaInterface pArea);
}

private double getMinPosition(Collection pAreas, PositionInterface pPosition)
{
double lMinPosition = Double.MAX_VALUE;
for (PageAreaInterface lArea : pAreas)
{
lMinPosition = Math.min(lMinPosition, pPosition.getPosition(lArea));
}
return lMinPosition;
}

private double getTop(Collection pAreas)
{
return getMinPosition(
pAreas,
new PositionInterface()
{
@Override
public double getPosition(PageAreaInterface pArea)
{
return pArea.getBoundingBox().getTop();
}
}
);
}

private double getLeft(Collection pAreas)
{
return getMinPosition(
pAreas,
new PositionInterface()
{
@Override
public double getPosition(PageAreaInterface pArea)
{
return pArea.getBoundingBox().getLeft();
}
}
);
}

private double getMaxPosition(Collection pAreas, PositionInterface pPosition)
{
double lMaxPosition = Double.MIN_VALUE;
for (PageAreaInterface lArea : pAreas)
{
lMaxPosition = Math.max(lMaxPosition, pPosition.getPosition(lArea));
}
return lMaxPosition;
}

private double getBottom(Collection pAreas)
{
return getMaxPosition(
pAreas,
new PositionInterface()
{
@Override
public double getPosition(PageAreaInterface pArea)
{
return pArea.getBoundingBox().getBottom();
}
}
);
}

private dou

Solution

Another alternative.

Nutshell

  • Don't put type into name (PageAreaInterface etc.)



  • Lose the Hungarian; appropriately-short methods remove its utility.



  • Create an enum and method to get specific coordinates.



  • Create getMinimum and getMaximum methods in a PageAreaCollection taking an enum.



Justifications (working backwards)

PageAreaCollection

Static utility methods strike me as un-OO, particularly when there are other options.

Create a type, with type-appropriate methods: code shrinks, and reads better:

min = PageAreaUtils.findMinimum(pageAreas, TOP); // Contrast with...
min = pageAreas.findMinimum(TOP);


Shorter is good, but which reads nicer? Which is more communicative?

With static imports, you're still left with:

min = findMinimum(pageAreas, TOP);    // Minimum pageAreas?! No...
min = findMinimumTop(pageAreas);      // Doesn't read right.
min = findMinimumTopIn(pageAreas);    // Better?
min = findMinimum(TOP).in(pageAreas); // Better?


IMO the amount of extra work/code to remove the method from its rightful place (a method of a pageArea collection) isn't worth the effort.

Enum in bounding box

The enum and utility method could live in the collection, too, if the bounding box class isn't yours to finagle.

// In bounding box, collection, or standalone.
public enum POSITION { TOP, LEFT, BOTTOM, RIGHT }

// In bounding box or collection.
public double getPosition(POSITION pos) {
    switch (pos) {
    case TOP:    return getTop();
    case LEFT:   return getLeft();
    case BOTTOM: return getBottom();
    case RIGHT:  return getRight();
    }
    throw new RuntimeException("Bad position provided: " + pos);
}


Min/max position locators in PageAreaCollection class

This is essentially the same as the previous suggestion to use Collections.min/max, but I'd still wrap it all up so the mainline code doesn't have to see how it's implemented. This way or that, it's significantly cleaner, with an appropriately-named Comparator.

Collection pageAreas;

public double getMinimumPosition(MyRect.POSITION pos) {
    double min = Double.MAX_VALUE;
    for (PageArea area : pageAreas) {
        min = Math.min(min, area.getBoundingBox().getPosition(pos));
    }
    return min;
}

public double getMaximumPosition(MyRect.POSITION pos) {
    double max = Double.MIN_VALUE;
    for (PageArea area : pageAreas) {
        max = Math.min(max, area.getBoundingBox().getPosition(pos));
    }
    return max;
}


Essentially the same if getPosition() needs to be in the collection. My quibble with having the method in the bounding box is that it makes getting the position a bit bulky; I'd actually prefer this:

area.getBoundingBox(pos) // or area.getBoundingBoxPosition(pos)?


Hungarian

Ew. A method that's a dozen lines long doesn't need differentiation between parameters and locals; it's obvious. At most I could see naming member variables, but even that... Meh.

Interface naming

A PageAreaInterface is just a PageArea. An implementation may deserve a special name, but it'd be a "special" PageArea in that it implements specific functionality likely worth naming. It's the same reason we don't name things IWhatever anymore. The interface is the Whatever, implementations provide specificity and deserve naming.

Code Snippets

min = PageAreaUtils.findMinimum(pageAreas, TOP); // Contrast with...
min = pageAreas.findMinimum(TOP);
min = findMinimum(pageAreas, TOP);    // Minimum pageAreas?! No...
min = findMinimumTop(pageAreas);      // Doesn't read right.
min = findMinimumTopIn(pageAreas);    // Better?
min = findMinimum(TOP).in(pageAreas); // Better?
// In bounding box, collection, or standalone.
public enum POSITION { TOP, LEFT, BOTTOM, RIGHT }

// In bounding box or collection.
public double getPosition(POSITION pos) {
    switch (pos) {
    case TOP:    return getTop();
    case LEFT:   return getLeft();
    case BOTTOM: return getBottom();
    case RIGHT:  return getRight();
    }
    throw new RuntimeException("Bad position provided: " + pos);
}
Collection<PageArea> pageAreas;

public double getMinimumPosition(MyRect.POSITION pos) {
    double min = Double.MAX_VALUE;
    for (PageArea area : pageAreas) {
        min = Math.min(min, area.getBoundingBox().getPosition(pos));
    }
    return min;
}

public double getMaximumPosition(MyRect.POSITION pos) {
    double max = Double.MIN_VALUE;
    for (PageArea area : pageAreas) {
        max = Math.min(max, area.getBoundingBox().getPosition(pos));
    }
    return max;
}
area.getBoundingBox(pos) // or area.getBoundingBoxPosition(pos)?

Context

StackExchange Code Review Q#6330, answer score: 4

Revisions (0)

No revisions yet.