Find intersecting rectangle

Given two rectangles, return the intersecting rectangles. This class is thread-safe. It takes rectangles in two different representations, and returns the intersecting rectangle in the respective representations.

Looking for code review, optimization and best practices.

```
final class RectangleWithPoints {
private final int leftTopX;
private final int leftTopY;

private final int rightBottomX;
private final int rightBottomY;

RectangleWithPoints (int leftTopX, int leftTopY, int rightBottomX, int rightBottomY) {
this.leftTopX = leftTopX;
this.leftTopY = leftTopY;
this.rightBottomX = rightBottomX;
this.rightBottomY = rightBottomY;
}

public int getTopLeftX() {
return leftTopX;
}

public int getTopLeftY() {
return leftTopY;
}

public int getBottomRightX() {
return rightBottomX;
}

public int getBottomRightY() {
return rightBottomY;
}

@Override
public String toString() {
/*
* http://stackoverflow.com/questions/1532461/stringbuilder-vs-string-concatenation-in-tostring-in-java
*/
return "TopLeftPoint: (" + leftTopX + "," + leftTopY + ")" + " BottomRightPoint: (" + rightBottomX + "," + rightBottomY + ")" ;
}

@Override
public int hashCode() {
final int prime = 31;
int result = 1;
result = prime * result + leftTopX;
result = prime * result + leftTopY;
result = prime * result + rightBottomX;
result = prime * result + rightBottomY;
return result;
}

@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null)
return false;
if (getClass() != obj.getClass())
return false;
RectangleWithPoints other = (RectangleWithPoints) obj;
if (leftTopX != other.leftTopX)
return false;
if (leftTopY != other.leftTopY)

Instead of putting unit tests in the main method, create proper JUnit tests.

At least one of your unit tests is incorrect:

assertEquals(new RectangleWithPoints(15, 25, 25, 5), 
        fecthIntersectingRectangle(new RectangleWithPoints(10,  35,  25, 15), 
                new RectangleWithPoints(15,  25,  35, 5)));

This test corresponds to something like this:

AAA
AAA
AXXBB
AXXBB
 XXBB
 XXBB

The intersection of the A and B rectangles (right side of your case) should be (15, 25, 25, 15) instead of (15, 25, 25, 5). So something's wrong with your intersection implementation.

Try to cover all corner cases with unit tests. For example test non-intersecting rectangles:

@Test(expected = IllegalArgumentException.class)
public void testNonIntersectingRectangles() {
    OverlappingRectangles.fetchIntersectingRectangle(new RectangleWithHeightAndWidth(10, 20, 20, 20),
            new RectangleWithHeightAndWidth(120, 130, 20, 20));
}

Similarly, adding unit tests for the intersects method would be good too.

The classes RectangleWithPoints and RectangleWithHeightAndWidth are not general enough. One could be transformed to the other, so it would be better to have a single Rectangle class, with factory methods like createWithPoints and createWithHeightAndWidth. That would eliminate a lot of your duplicated code, as only one equals, toString, hashCode and intersects would be enough.

Name your variables and methods consistently: either use leftTopX and getLeftTopX everywhere or topLeftX and getTopLeftX everywhere but don't mix both here and there.

About toString, the SO discussion you linked is concerning StringBuilder versus concatenation efficiency. In case of your toString methods, performance doesn't matter. You only need to optimize string concatenation in specific situations where performance really matters and the method is called millions of times. In toString methods like yours I use this less efficient but more readable format:

return String.format("TopLeftPoint: (%s,%s) BottomRightPoint: (%s,%s)",
        topLeftX, topLeftY, bottomRightX, bottomRightY);

A more common and compact form of your equals method would be:

@Override
public boolean equals(Object obj) {
    if (obj instanceof RectangleWithPoints) {
        RectangleWithPoints other = (RectangleWithPoints) obj;
        return leftTopX == other.leftTopX
                && leftTopY == other.leftTopY
                && rightBottomX == other.rightBottomX
                && rightBottomY == other.rightBottomY;
    }
    return false;
}

I would rename fecthIntersectingRectangle to getIntersectingRectangle. The word "fetch" usually evokes downloading something from a remote service or database.

It might make sense to move the methods of the OverlappingRectangles utility class inside your new refactored Rectangle. It depends on your use case.

See my refactored version on GitHub (taking hints from @david-harkness as well).

StackExchange Code Review Q#47029, answer score: 4