8
\$\begingroup\$

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
 points.add(((Square)s).getTopLeft());
 points.add(bottomLeft);
 points.add(topRight);
 points.add(bottomRight);
    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
 distance= distance +((Circle)s).getRadius();
 if(distance- this.getRadius()> Shape.TOLERANCE || distance==this.getRadius()){ //The method will return false if both circles have 0 radius
    flag=false;                                           //make flag false if the distance is more than the radius of the object Circle
 }
}

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

3 Answers 3

6
\$\begingroup\$

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>();
        points.add(((Square) s).getTopLeft());
        points.add(bottomLeft);
        points.add(topRight);
        points.add(bottomRight);
        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
                 || distance == this.getRadius()) {
            // 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());
        distance = distance + circle.getRadius();
        // The method will return false if both circles have 0 radius
        if (distance - this.getRadius() > Shape.TOLERANCE
                 || distance == this.getRadius()) {
            // make flag false if the distance is more than
            // the radius of the object Circle
            return false;
        }
    }

    return true;
}
\$\endgroup\$
0
5
\$\begingroup\$

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.

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

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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