Refactor function overloaded methods in just one method

Question

Currently I have a method overloading the following method:

    public boolean isHorizontalOrVertical(Point firstPoint, Point secondPoint) {
        return firstPoint.getFirstComponent() == secondPoint.getFirstComponent()
                || firstPoint.getSecondComponent() == secondPoint.getSecondComponent();
    }

    public boolean isHorizontalOrVertical(List<Point> points) {
        if (points == null || points.size() < 2) {
            throw new IllegalArgumentException("invalid number of points");
        }
        Point start = points.get(0);
        return points.stream()
                .allMatch(p -> isHorizontalOrVertical(start, p));
    }

The method is needed to check if two or three points are vertical/horizontal to each other. In the case of three points, it just has to check if the two last points are horizontal/vertical to the start point.

Does anyone have any idea how I can get it all into just one method?

You could use variadic arguments, you could have one function which takes a list? — PiRocks, Feb 22 '20 at 11:39
Yes, but how would one method look like with the required features? — , Feb 22 '20 at 11:41
Why would you want to do that? I would rename your second method, so that you can read it checks if **all** the points in the list are horizontal or vertical. something like: `allPointsAreHorizontalOrVertical` — Willem, Feb 22 '20 at 11:45
What is the advantage of requiring the caller to pass two points as a list? If you don't want to expose the two-point version in a public API, just make the method private. — kaya3, Feb 22 '20 at 12:40

score 1 · Answer 1 · edited Jun 20 '20 at 09:12

First and foremost I have to note the fact that it doesn't make sense, to me at least, a method which calculates if two entities are horizontal or vertical and those entities are Points. How can two points be horizontal or vertical?

isHorizontalOrVertical is a bad name

Overcoming the above, you could create a single method which calculates if two points are horizontal or vertical.

Change the name isHorizontalOrVertical because it's redundant. A better name is isHorizontal or isVertical. The method will return a boolean so if isHorizontal returns false, then it's vertical and vice versa. Probably a better name could be areTwoPointsHorizontal but I am having trouble even writing that because it transmits the wrong message, but feel free to choose your own.

So the method,

    public boolean isHorizontal(Point first, Point second){
        boolean sameFirstComponents = firstPoint.getFirstComponent() == 
                secondPoint.getFirstComponent();
        boolean sameSecondComponents = firstPoint.getSecondComponent() == 
                secondPoint.getSecondComponent();          
        return sameFirstComponents || sameSecondComponents;
    }

Finally, create a method which calculates if an arbitary number of points in a list are all between them horizontal or vertical (assuming if point A is horizontal with point B, then if point C is horizontal with B, so is with A).

Oveload that method since it does the same thing and the only thing changing are the parameters. (Note the use of the simple isHorizontal method from above)

   public boolean isHorizontal(List<Point> points){
        boolean allPointsHorizontal = true;
        for (int i=0; i<points.size(); i++) {
            
            boolean nextPointExists = i<points.size() - 1;
            if (nextPointExists) {
                Point current = points.get(i);
                Point next = points.get(i+1);
                allPointsHorizontal = allPointsHorizontal && isHorizontal(current,next);

                if (!allPointsHorizontal)
                    return false;  
            }
        }
        
        return allPointsHorizontal;
    }

Sweeper · Answer 2 · 2020-02-22T12:25:22.550

The method could be implemented like this:

public boolean isHorizontalOrVertical(Point firstPoint, Point secondPoint, Point thirdPoint) {
    return isHorizontalOrVertical(Arrays.asList(firstPoint, secondPoint, thirdPoint));
}

Your isHorizontalOrVertical(List<Point>) method will fail when the list is empty, and the call doesn't make much sense when the list has only one element.

I think a better way to do this is with two required parameters, plus a variadic parameter, so that callers must at least give 2 points.

private boolean are2PointsHorizontalOrVertical(Point firstPoint, Point secondPoint) {
    return firstPoint.getFirstComponent() == secondPoint.getFirstComponent()
            || firstPoint.getSecondComponent() == secondPoint.getSecondComponent();
}

public boolean arePointsHorizontalOrVertical(Point point1, Point point2, Point... rest) {
    return are2PointsHorizontalOrVertical(point1, point2) &&
        Arrays.stream(rest).allMatch(x -> are2PointsHorizontalOrVertical(point1, x));
}

This technically is still "one method" as far as the public interface is concerned. You can substitute the helper are2PointsHorizontalOrVertical back into the public method if you really want, but I don't see any benefit in doing that.

Arvind Kumar Avinash · Accepted Answer · 2020-02-22T12:45:31.093

0

You can have just one method as follows:

public boolean isHorizontalOrVertical(List<Point> points) {
    if (points == null || points.size() < 2) {
        throw new IllegalArgumentException("invalid number of points");
    }
    if (points.size() == 2) {
       return points.get(0).getFirstComponent() == points.get(1).getFirstComponent()
                || points.get(0).getSecondComponent() == points.get(1).getSecondComponent();
    } 
    Point start = points.get(0);
    return points.stream()
                .allMatch(p -> isHorizontalOrVertical(List.of(start, p)));
}

Note: If you are not using Java version >= 9, please use Arrays.asList instead of List.of.

edited Feb 22 '20 at 12:45

answered Feb 22 '20 at 12:06

Arvind Kumar Avinash

71,965
6
74
110

How is `return points.stream() .allMatch(p -> isHorizontalOrVertical(start, p));` supposed to work when there is just one method but it needs two arguments? – Feb 22 '20 at 12:12
What do you mean by `List.of(..)`? – Feb 22 '20 at 12:37
@EugenCovaci - please check https://stackoverflow.com/questions/46579074/what-is-the-difference-between-list-of-and-arrays-aslist – Arvind Kumar Avinash Feb 22 '20 at 12:37
Are you sure the OP is using Java version >= 9? – Feb 22 '20 at 12:39
@EugenCovaci - That's the assumption. If it is not the case, OP will comment. – Arvind Kumar Avinash Feb 22 '20 at 12:42
I am using Java 8 – Feb 22 '20 at 14:16
@ArvindKumarAvinash Thank you. Would it be better to use `if (points.isEmpty() || ...)` instead of `if (points == null || ...)`? – Feb 22 '20 at 18:41
It's not only better rather important to use `points == null` because `points.isEmpty()` will throw `NullPointerException` if `points` is `null`. – Arvind Kumar Avinash Feb 22 '20 at 18:48

score 0 · Answer 4 · answered Feb 22 '20 at 13:00

Actually, you can have only one method:

public boolean isHorizontalOrVertical(Point firstPoint, Point secondPoint, Point... others) {
    // check firstPoint, secondPoint for null is ommited
    if (others == null || others.length == 0) {
        return firstPoint.getFirstComponent() == secondPoint.getFirstComponent()
                || firstPoint.getSecondComponent() == secondPoint.getSecondComponent();
    } else {
        // First, create a stream with second point + others elements
        // then match against the first point
        return Stream.of(new Point[]{secondPoint}, others).flatMap(e -> Stream.of(e))
                .allMatch(p -> isHorizontalOrVertical(firstPoint, p));
    }

}

Refactor function overloaded methods in just one method

4 Answers4

isHorizontalOrVertical is a bad name