Simple random falling object animation in Java

Question

I am new to programming but I tried to make my code as readable as possible! Hopefully you can see what it does before you run the program. I did not implement an ups or fps controller, the animation may be fast for some computers and slower for others.

import javax.swing.*;
import java.awt.*;
import java.util.Random;

public class App extends JPanel {

    //changing these values will change the size of the game, while still remaining functional
    //within the size limit specified.
    static int windowWidth = 1300;
    static int windowHeight = 800;

    int randNumb = 0;
    int squareWidth = 25;
    int squareHeight = 25;
    int squareYLocation = -squareHeight;
    boolean numberCreated = false;
    static boolean gameRunning = false;

    //generates a random Y value inside the window for the square to spawn at
    public void generateRandomNumber() {
        Random rand = new Random();
        randNumb = rand.nextInt(windowWidth - squareWidth);
        numberCreated = true;
    }

    //paints a black screen, then paints a rectangle on top of the black screen
    public void paint(Graphics g) {
        g.setColor(Color.black);
        g.fillRect(0, 0, windowWidth, windowHeight);
        g.setColor(Color.BLUE);
        g.fillRect(randNumb, squareYLocation, squareWidth, squareHeight);
    }

    public void update() {

        //calls the generateRandomNumber() method which gives the square a random x value inside the screen
        if (!numberCreated) {
            generateRandomNumber();
        }
        //moves the squares y coordinate towards the bottom of the screen and stops once it hits the bottom
        if (squareYLocation <= windowHeight) {
            squareYLocation++;

            //resets the x and y location to a new position
        } else {
            numberCreated = false;
            squareYLocation = -squareHeight;
        }
    }

    //sets the while loop to true to start the game
    public void start() {
        gameRunning = true;
    }

    public static void main(String[] args) throws InterruptedException {

        App game = new App();
        JFrame frame = new JFrame();
        frame.add(game);
        frame.setVisible(true);
        frame.setSize(windowWidth, windowHeight);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setTitle("Raining Squares");
        frame.setResizable(false);
        frame.setLocationRelativeTo(null);

        game.start();

        //updates square position, repaints square, and slows down update and paint speed.
        while (gameRunning) {
            game.update();
            game.repaint();
            Thread.sleep(1);
        }
    }
}

Answer 1

Here is my list of possible improvements:

• Constants should be made final and if possible also private

  private static final int WINDOW_WIDTH = 1300;
  private static final int WINDOW_HEIGHT = 800;
  private static final int SQUARE_WIDTH = 25;
  private static final int SQUARE_HEIGHT = 25;
  

• Class members should remain private, if accessible from other classes use setters/getters

  private int randNumb = 0;
  private int squareYLocation = -SQUARE_HEIGHT;
  private boolean numberCreated;
  private boolean gameRunning;
  

'

• Limit class methods visibility, if no other class uses them

  private void generateRandomNumber() {
      Random rand = new Random();
      randNumb = rand.nextInt(windowWidth - squareWidth);
      numberCreated = true;
  }
  
  private void paint(Graphics g) {
      g.setColor(Color.black);
      g.fillRect(0, 0, windowWidth, windowHeight);
      g.setColor(Color.BLUE);
      g.fillRect(randNumb, squareYLocation, squareWidth, squareHeight);
  }
  

• Don't use one letter names for object instances, use the full name or give an appropriate name that is easy to understand or reflects something relevant.

  private void paint(Graphics graphics) {
      graphics.setColor(Color.black);
      graphics.fillRect(0, 0, windowWidth, windowHeight);
      graphics.setColor(Color.BLUE);
      graphics.fillRect(randNumb, squareYLocation, squareWidth, squareHeight);
  }
  

• Consistent use of Whitespace, it ok to add extras white lines but be consistent about it. Eg. update/main have a whiteline, other methods don't.

• The naming of numberCreated was a bit confusing, I first thought it was only generated once but after looking through the code I saw that the flag could be set to false again. I would suggest changing to a different name eg invalidateNumber or something else that makes it more clear.

• App extends JFrame is not a good name for the class, GameFrame extends JFrame would be better. And even better would be the use of the composition pattern to have an App class. (You can separate your game logic from its View (MVC)).

• Commenting style is not really conform the guidelines. Your style for inline comments is ok. But normally method comments are something like this:

   /**
    * Your comment goes here, also use 
    * @param for your params
    */
   public void paint(Graphics g) {
       g.setColor(Color.black);
       g.fillRect(0, 0, windowWidth, windowHeight);
       g.setColor(Color.BLUE);
       g.fillRect(randNumb, squareYLocation, squareWidth, squareHeight);
   }
  

Answer 2

In terms of readability, this is fine, nicely written, well-formatted, good code.

These variables look like constants:

static int windowWidth = 1300;
static int windowHeight = 800;

int squareWidth = 25;
int squareHeight = 25;

It's good to make that perfectly clear:

private static final int WINDOW_WIDTH = 1300;
private static final int WINDOW_HEIGHT = 800;

private static final int SQUARE_WIDTH = 25;
private static final int SQUARE_HEIGHT = 25;

In this code:

public void generateRandomNumber() {
  Random rand = new Random();

Do you really need a new Random() in every call? It will be more efficient to reuse a single Random object in a field.

About this:

static boolean gameRunning = false;

Does this really need to be a static variable? If you can, it would be better to move the game loop in a member function so that this variable too can be a member instead of static. (static things in general are not great, unless constants.)

Answer 3

Here's some things I spotted:

public class App extends JPanel {

App is not a good name because it can be any kind of app. It doesn't tell the reader anything about the program. Consider changing it to "GamePanel" or "FallingRectangleApp".

static int windowWidth = 1300;
static int windowHeight = 800;

These are constants and should be written in upper-case, they should also be made final. As a rule, if you can make something a constant, you should.

int squareWidth = 25;
int squareHeight = 25;

What you're creating does not have to be a square, it can be a rectangle, since you're using two different constants for height and width. It's a little nitpicky, but it pays to be precise. Especially when others read your code later. So either rename your variables to "rectangleWidth" and the like, or use only one variable (eg. int squareLength = 25;)

//generates a random Y value inside the window for the square to spawn at
public void generateRandomNumber() {
    Random rand = new Random();
    randNumb = rand.nextInt(windowWidth - squareWidth);
    numberCreated = true;
}

Don't create a new Random object every time you need a new random number. Random() without seed is seeded with the current nano-time, so if this method is called twice with the same time, it will two random objects that will produce the same numbers. Instead create one Random object and use that the whole time.

//paints a black screen, then paints a rectangle on top of the black screen
public void paint(Graphics g) {
    g.setColor(Color.black);
    g.fillRect(0, 0, windowWidth, windowHeight);
    g.setColor(Color.BLUE);
    g.fillRect(randNumb, squareYLocation, squareWidth, squareHeight);
}

The two colors you're using can be made into constants as well (Since that's what they are right now).

Be consistent in your use of the color constants. Use all capitals or not, don't mix it up (use either Color.Blue or Color.BLUE, not both). Not super important, but useful for readability.

//calls the generateRandomNumber() method which gives the square a random x value inside the screen
if (!numberCreated) {
    generateRandomNumber();
}

The numberCreated flag is not nessesary with some small rewrites. When you find these kinds of state flags be sure to check if they're really nessesary. They can be a cause for confusion when reading code.

if (squareYLocation <= windowHeight) {
    squareYLocation++;

    //resets the x and y location to a new position
}

It's not clear to me that this comment is for the else block. Move it into the else block if it was meant there.

else {
    numberCreated = false;
    squareYLocation = -squareHeight;
}

Instead of setting the numberCreated flag, just call generateRandomNumber(); in the else block.

//sets the while loop to true to start the game
public void start() {
    gameRunning = true;
}

The gameRunning variable is not really nessesary, since you're not changing it anywhere yet.

If you decide to remove the numberCreated flag, be sure to call generateRandomNumber(); at the start as well.

while (gameRunning) {
    game.update();
    game.repaint();
    Thread.sleep(1);
}        

Thread.sleep(1) is very short, and means that the thread sleeps for only 1 millisecond.

Future options:

I think your next challenge should be to create this same program, but with multiple falling rectangles, instead of just one. This would be a good time to read up on Object Oriented Programming, which will allow you to create a FallingRectangle Object which can take on part of the responsibilities.

Anyways, good job on this one, and I hope this review helps!

EDIT: Just a small response to Krzysztofs comment:

Funny enough, I already started making multiple squares fall! I started it after I made this post. Eventually It is going to be a dodge the meteors type game. Thanks for your feedback. (my computer is quite slow, 1 millisecond slowed down my program a lot)

It makes a lot of sense that it slows down your program since the other two calls are probably faster then 1 millisecond. The problem is that your screen cannot keep up with a refresh rate so high. It can't refresh itself a 1000 times per second, so why redraw that often?

The standard refresh rate of games is around 60 fps, which is about 16 milliseconds per refresh, or Thread.sleep(16); if you neglect the time it takes for the other calls(which you shouldn't do, but let's ignore that for now).