Check if one shape is in another shape

I have a code to check if one shape (circle or square) envelops another. I have used this code. It gives true if the this object envelops the passed shape. Can anyone suggest me an improvement or better alternate method?

public boolean envelops(Shape s) {
boolean flag = true;
double distance = 0.0;
if(s instanceof Square){ //check if the shape is a Square
Point bottomLeft= new Point(((Square)s).getTopLeft().getX(),((Square)s).getTopLeft().getY()-((Square)s).getSideLength()); //calculating the cordinates of the square
Point bottomRight= new Point(((Square)s).getTopLeft().getX()+((Square)s).getSideLength(),((Square)s).getTopLeft().getY()-((Square)s).getSideLength());
Point topRight= new Point(((Square)s).getTopLeft().getX()+  ((Square)s).getSideLength(),((Square)s).getTopLeft().getY());

ArrayList<Point> points = new ArrayList<Point>(); //storing points in a ArrayList of points
for(int i=0; i<points.size(); i++)
{
distance= this.center.distance(points.get(i));  //finding ditance of each cordinate from the center of the circle
if(distance-this.radius>Shape.TOLERANCE) flag= false;      //if the distance is greater than the radius then flag becomes false, ie., the cordinate is outside the circle

}
}

if(s instanceof Circle){
distance=this.center.distance(((Circle)s).getCenter());  //finding distance between the centers of the 2 circles
flag=false;                                           //make flag false if the distance is more than the radius of the object Circle
}
}

return flag;
}

• Welcome to Code Review! Good job on your first post. Oct 31, 2015 at 18:56
• What happens when you want to see if a Circle envelops a Rectangle? Oct 31, 2015 at 19:33
• Isn't that missing a bunch of code? i.e., the full code for Circle, Shape, Square, etc. Oct 31, 2015 at 21:21

Your indentation is quite messy. Indentation is usually 4 spaces in Java, so use 4 spaces.

Including some other minor readability improvements (such as each line is a maximum of 80 characters), your code will look like:

public boolean envelops(Shape s) {
boolean flag = true;
double distance = 0.0;
if (s instanceof Square) { // check if the shape is a Square
// calculating the coordinates of the square
Point bottomLeft = new Point(((Square) s).getTopLeft().getX(),
((Square) s).getTopLeft().getY() - ((Square) s).getSideLength());
Point bottomRight = new Point(((Square) s).getTopLeft().getX() +
((Square) s).getSideLength(),
((Square) s).getTopLeft().getY() - ((Square) s).getSideLength());
Point topRight = new Point(((Square) s).getTopLeft().getX() +
((Square) s).getSideLength(), ((Square) s).getTopLeft().getY());

// storing points in a ArrayList of points
ArrayList<Point> points = new ArrayList<Point>();
for (int i = 0; i < points.size(); i++) {
//finding ditance of each cordinate from the center of the circle
distance = this.center.distance(points.get(i));
// if the distance is greater than the radius then flag becomes false,
// ie., the cordinate is outside the circle
if (distance - this.radius > Shape.TOLERANCE) flag = false;
}
}

if (s instanceof Circle) {
// finding distance between the centers of the 2 circles
distance = this.center.distance(((Circle) s).getCenter());
distance = distance + ((Circle) s).getRadius();
// The method will return false if both circles have 0 radius
if (distance - this.getRadius() > Shape.TOLERANCE
// make flag false if the distance is more than
// the radius of the object Circle
flag = false;
}
}

return flag;
}


Some other points:

You don't need to cast to Square or Circle every time. Instead, create a new Square or Circle:

Square square = (Square) s;


and:

Circle circle = (Circle) s;


You don't need to keep a flag that you will eventually return with. Just return directly.

You don't need an ArrayList: just use an array.

Final Code

public boolean envelops(Shape s) {
double distance = 0.0;
if (s instanceof Square) { // check if the shape is a Square
Square square = (Square) s;
// calculating the coordinates of the square
Point bottomLeft = new Point(square).getTopLeft().getX(),
square.getTopLeft().getY() - square.getSideLength());
Point bottomRight = new Point(square.getTopLeft().getX() +
square.getSideLength(),
square.getTopLeft().getY() - square.getSideLength());
Point topRight = new Point(square.getTopLeft().getX() +
square.getSideLength(), square.getTopLeft().getY());

// storing points in a ArrayList of points
Point[] points = { square.getTopLeft(), bottomLeft, topRight, bottomRight };
for (int i = 0; i < points.length; i++) {
//finding distance of each coordinate from the center of the circle
distance = this.center.distance(points[i]);
// if the distance is greater than the radius then flag becomes false,
// ie., the cordinate is outside the circle
if (distance - this.radius > Shape.TOLERANCE) return false;
}
}

if (s instanceof Circle) {
Circle circle = (Circle) s;
// finding distance between the centers of the 2 circles
distance = this.center.distance(circle.getCenter());
// The method will return false if both circles have 0 radius
if (distance - this.getRadius() > Shape.TOLERANCE
// make flag false if the distance is more than
// the radius of the object Circle
return false;
}
}

return true;
}


Your code is doomed to become larger and larger as more shapes get added, you need a completely different method.

Each shape should be able to output its edgePoints, its "corners" (the circle will have to do a small approximation), after you got this method, checking if a point is inside a Shape is trivial (pseudo-code is given):

class Point {
public boolean isInside(Shape s) {
ArrayList<Point> points = s.edgePoints()
return this.x >= points.map(getX).min && \
this.x <= points.map(getX).max && \
this.y >= points.map(getX).min && \
this.y <= points.map(getX).min

}
}


A shape a envelops another shape b if all the points of b are inside a.

• As long as you don't add concave shapes. Oct 31, 2015 at 21:58
• @RemcoGerlich concave shapes may be divided in n convex shapes and then checked if the point is inside any of them Oct 31, 2015 at 22:14

The big issue here is the way you're using instanceof to differentiate cases inside your method; this is what polymorphism is for. To expand on @Caridorc's answer, doing this will eventually balloon that method into an unmaintainable mess and also prevent client code from effectively subclassing Shape.

Instead, I would add the following methods to Shape:

• Point[] getCornerPoints() (returns an array or other structure containing the extreme points on that shape)
• bool containsPoint(Point p)

Now you can write your method generically as:

bool envelops(Shape s) {
for(Point p: s.getCornerPoints()) {
if(!this.containsPoint(p)) { return false; }
}
return true;
}


This way it is up to each subclass of Shape to fill in the smallest possible units of behavior to correctly implement your method, and most importantly, Shape never has to know what its subclasses are.