# Generating 50 Random Shapes

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:

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
// 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 {

// Constructor is the same as AbstractShape
public Circle(double xpos, double ypos) {
super(xpos, ypos);
}

}

}

// 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);
return theClone;
}

// Check to see if 2 objects are the same
public boolean equals(Circle c) {
&& c.getXPos() == this.xpos
&& c.getYPos() == this.ypos;
}

// Specific toString() method
public String toString() {
String message = "\n[CIRCLE]"
+ "\nPosition: (" + xpos + ", " + ypos
+ "\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;
}
}

• This is pretty well done already. I have only 1 suggestion, see my answer. Nov 24, 2016 at 21:43

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

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.

### 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

• 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 Nov 25, 2016 at 1:30

## 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.

    // Set radius value. Must be positive!

if (r < 0.0) {
throw new IllegalArgumentException("Radius cannot be negative!");
}