patternjavaMinor
Interface for obtaining the bounding box for a collection of elements
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
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
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:
Shorter is good, but which reads nicer? Which is more communicative?
With static imports, you're still left with:
IMO the amount of extra work/code to remove the method from its rightful place (a method of a
The
Min/max position locators in
This is essentially the same as the previous suggestion to use
Essentially the same if
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
Nutshell
- Don't put type into name (
PageAreaInterfaceetc.)
- Lose the Hungarian; appropriately-short methods remove its utility.
- Create an
enumand method to get specific coordinates.
- Create
getMinimumandgetMaximummethods in aPageAreaCollectiontaking anenum.
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 boxThe
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 classThis 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.