Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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<PageAreaInterface> 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<PageAreaInterface> pAreas)
{
    return getMinPosition(
                pAreas, 
                new PositionInterface() 
                {
                    @Override
                    public double getPosition(PageAreaInterface pArea)
                    {
                        return pArea.getBoundingBox().getTop();
                    }
                }
    );
}

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

private double getMaxPosition(Collection<PageAreaInterface> 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<PageAreaInterface> pAreas)
{
    return getMaxPosition(
                pAreas, 
                new PositionInterface() 
                {
                    @Override
                    public double getPosition(PageAreaInterface pArea)
                    {
                        return pArea.getBoundingBox().getBottom();
                    }
                }
    );
}

private double getRight(Collection<PageAreaInterface> pAreas)
{
    return getMaxPosition(
                pAreas, 
                new PositionInterface() 
                {
                    @Override
                    public double getPosition(PageAreaInterface pArea)
                    {
                        return pArea.getBoundingBox().getRight();
                    }
                }
    );
}
share|improve this question

2 Answers 2

up vote 3 down vote accepted

Another alternative.

Nutshell

  1. Don't put type into name (PageAreaInterface etc.)
  2. Lose the Hungarian; appropriately-short methods remove its utility.
  3. Create an enum and method to get specific coordinates.
  4. 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<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;
}

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.

share|improve this answer
    
Cool. Thanks for your thoughts. It is not shown in the code but all those static functions are in a certain builder class - but you are definitely right, they should be moved to their appropriate object, I like the getBoundingBoxPosition (so that getBoundingBox still returns a box). I agree with the hungarian part (code convention policy). The "do not put type" is a new one to me - I think I like it, thanks! One thing though, if no type then why the PageAreaCollection? (instead of e.g. PageAreas?) –  j-a Nov 27 '11 at 17:19
    
@j-a No reason; just what I happened to use--you're right, it's probably redundant, particularly if the collection type is unspecified. –  Dave Newton Nov 27 '11 at 17:25

It looks fine. Downvote me, but if there is no more possible comparisons I would write four simple for loops instead of the four inner classes and the interface. I think it would be a little bit more readable and mean less code.

Another possibility is creating four Comparators and calling Collections.min() and Collections.max(). It could be an abstract PareAreaComparator with four subclasses:

public abstract class PareAreaComparator implements Comparator<PageAreaInterface> {

    public PareAreaComparator() {
    }

    @Override
    public int compare(final PageAreaInterface o1, final PageAreaInterface o2) {
        final BouningBox boundingBox1 = o1.getBoundingBox();
        final BouningBox boundingBox2 = o2.getBoundingBox();
        return compare(boundingBox1, boundingBox2);
    }

    protected abstract int compare(BouningBox boundingBox1, BouningBox boundingBox2);
}

Everyone (should) know Comparators, so newcomers are probably more familiar with them than a custom interface which means a little bit easier maintenance.

share|improve this answer
1  
I see what you mean. The reason why I went for the inner classes and the interface was more to reduce code-logic-duplication rather than code-line-duplication. When the logic is this simple though, I think I agree with you. Thanks –  j-a Nov 27 '11 at 13:39

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.