Generating 50 Random Shapes

Question

I am a beginner Java programmer. I have just finished up an assignment and would appreciate some advice and/or constructive criticism on my program. For context, I have the assignment details, followed by the full code below:

The Shape interface is as follows:

public interface Shape {
 public double area();
 public double perimeter();
 public double getXPos();
 public double getYPos();
 public void move(double xLoc, double yLoc);
 public String toString();
}

An AbstractShape has the following attributes:

• xPos
• yPos

An AbstractShape has the following behaviour:

• move(x,y)
• getXPos
• getYPos
• toString

A Circle adds the following attributes:

• radius

A Circle adds the following behaviour:

• area
• perimeter
• equals(Circle)
• clone
• updated toString

A Rectangle adds the following attributes:

• height
• width

A Rectangle adds the following behaviour:

• area
• perimeter
• equals(Rectangle)
• clone
• updated toString

Also include constructors in your classes.

Write a test harness which generates 50 random Shapes, stores them in an array, then prints them out.

Main Class: RandomShapes

public class RandomShapes {
 public static void main(String[] args) {
 // Creating the random variables
 double randX, randY, randRadius, randWidth, randHeight;
 // Array to store AbstractShapes
 AbstractShape[] shapes = new AbstractShape[50];
 for (int i = 0; i < shapes.length; i++) {
 // Random number between 1-2 to decide rectangle, or circle
 int circleOrRect = (int) (Math.random() * 2 + 1);
 // Creating 6 random numbers
 randX = (int) ((Math.random() * 100) + 1);
 randY = (int) ((Math.random() * 100) + 1);
 randRadius = (int) ((Math.random() * 100) + 1);
 randWidth = (int) ((Math.random() * 100) + 1);
 randHeight = (int) ((Math.random() * 100) + 1);
 // If random number is 1, create/add a circle to array, else, add rectangle
 if (circleOrRect == 1) {
 // Create new circle with random (x, y)
 shapes[i] = new Circle(randX, randY);
 // Cast AbstractShape to Circle and set random radius
 ((Circle) shapes[i]).setRadius(randRadius);
 // Print out the object's info
 System.out.println("\n#" + (i+1) + shapes[i].toString());
 } else {
 // Create new rectangle with random (x, y)
 shapes[i] = new Rectangle(randX, randY);
 // Cast AbstractShape to Rectangle and set random height/width
 ((Rectangle) shapes[i]).setHeight(randHeight);
 ((Rectangle) shapes[i]).setWidth(randWidth);
 // Print out the object's info
 System.out.println("\n#" + (i+1) + shapes[i].toString());
 }
 }
 }
}

Interface: Shape

public interface Shape {
 // All the abstract methods
 public double area();
 public double perimeter();
 public double getXPos();
 public double getYPos();
 public void move(double xLoc, double yLoc);
 public String toString();
}

AbstractClass: AbstractShapes

abstract public class AbstractShape implements Shape {
 // X and Y positions
 protected double xpos;
 protected double ypos;
 // Constructor for an AbstractShape
 public AbstractShape(double x, double y) {
 xpos = x;
 ypos = y;
 }
 // To move a shape
 public void move(double xLoc, double yLoc) {
 xpos = xLoc;
 ypos = yLoc;
 }
 // Get X position of a shape
 public double getXPos() {
 return xpos;
 }
 // Get Y position of a shape
 public double getYPos() {
 return ypos;
 }
 //Generic toString method
 public String toString() {
 String message = "xPos: " + xpos
 + "\nyPos: " + ypos;
 return message;
 }
}

Sub-Class: Circle

public class Circle extends AbstractShape {
 // Radius variable
 private double radius;
 // Constructor is the same as AbstractShape
 public Circle(double xpos, double ypos) {
 super(xpos, ypos);
 }
 // Set radius value
 public void setRadius(double r) {
 radius = r;
 }
 // Get radius value
 public double getRadius() {
 return radius;
 }
 // Calculate area
 public double area() {
 double area = Math.PI * Math.pow(radius, 2);
 return area;
 }
 // Calculate perimeter (circumference)
 public double perimeter() {
 double perimeter = 2 * Math.PI * radius;
 return perimeter;
 }
 // Clone the object
 public Circle clone() {
 Circle theClone;
 theClone = new Circle(this.xpos, this.ypos);
 theClone.setRadius(this.radius);
 return theClone;
 }
 // Check to see if 2 objects are the same
 public boolean equals(Circle c) {
 return c.radius == this.radius
 && c.getXPos() == this.xpos
 && c.getYPos() == this.ypos;
 }
 // Specific toString() method
 public String toString() {
 String message = "\n[CIRCLE]"
 + "\nPosition: (" + xpos + ", " + ypos
 + ")\nRadius: " + radius
 + "\nArea: " + this.area()
 + "\nCircumference: " + this.perimeter();
 return message;
 }
}

Sub-Class: Rectangle

public class Rectangle extends AbstractShape {
 // Height and width variables
 private double height;
 private double width;
 // Constructor for rectangle (same as super)
 public Rectangle(double xpos, double ypos) {
 super(xpos, ypos);
 }
 // Set height value
 public void setHeight(double h) {
 height = h;
 }
 // Get the height value
 public double getHeight() {
 return height;
 }
 // Set the width value
 public void setWidth(double w) {
 width = w;
 }
 // Get the width value
 public double getWidth(double w) {
 return width;
 }
 // Calculate the area
 public double area() {
 double area = height * width;
 return area;
 }
 // Calculate the perimeter
 public double perimeter() {
 double perimeter = (2 * width) + (2 * height);
 return perimeter;
 }
 // Clone the object
 public Rectangle clone() {
 Rectangle theClone;
 theClone = new Rectangle(this.xpos, this.ypos);
 theClone.setHeight(this.height);
 theClone.setWidth(this.width);
 return theClone;
 }
 // Check to see if 2 objects are the same
 public boolean equals(Rectangle r) {
 return r.height == this.height
 && r.width == this.width
 && r.getXPos() == this.xpos
 && r.getYPos() == this.ypos;
 }
 // Specific toString() method
 public String toString() {
 String message = "\n[RECTANGLE]"
 + "\nPosition: (" + xpos + ", " + ypos
 + ")\nHeight: " + height
 + "\nWidth: " + width
 + "\nArea: " + this.area()
 + "\nPerimeter: " + this.perimeter();
 return message;
 }
}

• Tamoghna Chowdhury: This is pretty well done already. I have only 1 suggestion, see my answer.

Answer 1

I'll just review RandomShapes. I figure there's not much to go wrong when you are given a design that clear:

Comments

You're overcommenting. This is one of the things that annoys me most when reading code that's written for assignments. The comments have been written for the sake of there being comments. That's absolutely not the point of comments. Comments that merely restate what's already written in code are useless, if not dangerous.

I strongly advise you to not use comments that way.

Use of random

You're repeatedly using Math.random(). Since you're using it to get a random number in the range of [1..100] you may want to check into using the Random object from the java library.

Consider the following code:

Random rng = new Random();
randX = rng.nextInt(100) + 1;
randY = rng.nextInt(100) + 1;
// ...

This removes all the casts from your random parameter creation.

Variable Scopes and Types

The random variables are declared outside the for-loop, but not used after. It's generally best practice to declare variables as close as possible to their usage and in the smallest scope possible.

This allows you to keep the enclosing scope clean and eases programming, especially in longer blocks.

Additionally using an int to store a binary choice (between circle and rectangle) is not a good idea. Since you only have Circles and Rectangles, you should consider storing circleOrRect in a boolean. You can obtain a random boolean from a Random object by invoking nextBoolean()

One additional thing. The code directly stores the shapes it creates in the shapes array. This makes it needlessly difficult to set the interesting values of radius or width and height respectively.

One option to fix this would be to introduce constructors to the subclasses that take all the relevant arguments as parameters. Consider:

if (circle) {
 shapes[i] = new Circle(randX, randY, randRadius);
} else {
 shapes[i] = new Rectangle(randX, randY, randWidth, randHeight);
}

Code duplication and placement

One last thing: When two branches of code do the same thing, one would strive to merge them. When two branches of code need different things, one would strive to only generate the resources necessary for the code when it's known what will be necessary.

I'd rewrite the for-loop as follows:

for (int i = 0; i < shapes.length; i++) {
 boolean circle = rng.nextBoolean();
 int randX = rng.nextInt(100) + 1;
 int randY = rng.nextInt(100) + 1;
 if (circle) {
 int randRadius = rng.nextInt(100) + 1;
 shapes[i] = new Circle(randX, randY, randRadius);
 } else {
 int randWidth = rng.nextInt(100) + 1;
 int randHeight = rng.nextInt(100) + 1;
 shapes[i] = new Rectangle(randX, randY, randWidth, randHeight);
 }
 System.out.println("\n#" + (i + 1) + shapes[i].toString());
}

This minimizes the effort expended and still accomplishes the same result

• jzbakos: For some reason, I assumed since we needed a setter method, it needed to be used. Also, I realize the comments are extremely overused, however, that is what my teacher requested from us to ensure we understand what we are doing. Thanks a ton. +1

Answer 2

Reusability and code safety

Since a lot has already been covered, I will focus on defensive programming to make your code safer, especially safer to reuse.

Let's consider Circle and its constructor and method:

public Circle(double xpos, double ypos)

public void setRadius(double r)

You declare this class to be public. That means anyone can create a Circle instance. You never know what kind of things people will try to stuff into your constructor, but you want your Circle to be safe and correct.

What happens if someone passes a negative radius? You will and up with an illegal circle. That is something you want to prevent.

Consider this addition:

 // Set radius value. Must be positive!
 public void setRadius(double r) {
 if (r < 0.0) { 
 throw new IllegalArgumentException("Radius cannot be negative!"); 
 }
 radius = r;
 }

This makes sure your classes stay valid, and are better reusable by others.

• jzbakos: I've actually used the advice from the answer above yours and moved the radius into the constructor itself. Would I simply place your code into the constructor, instead of the setter method?
• Rob Audenaerde: Yes. You want to prevent construction of an illegal object.

Answer 3

Suggestions

You have written pretty good code already, and your describe your methods with comments just before them. You could make those comments into property JavaDoc comments, see the Oracle JavaDoc tutorial here.