CODEWITHSUNDEEP
javaoverloading
user12881831
user12881831

Reputation:

Refactor function overloaded methods in just one method

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?

Upvotes: 1

Views: 244

Answers (4)

Themelis
Themelis

Reputation: 4255

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;
    }

Upvotes: 1

user10871691
user10871691

Reputation:

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));
    }

}

Upvotes: 0

Arvind Kumar Avinash
Arvind Kumar Avinash

Reputation: 79435

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.

Upvotes: 0

Sweeper
Sweeper

Reputation: 273105

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.

Upvotes: 0

Related Questions