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:

• 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() * 10

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

StackExchange Code Review Q#148029, answer score: 2