5
\$\begingroup\$

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 a ups or fps controller, the animation may be fast for some computers and slower for others. I also tried to use the information provided to me from my first code review. Eventually I will be making this concept into a dodge the meteors type game.

Game class:

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

public class Game extends JPanel {

    //changing these values will change the size of the game, while still remaining functional
    //within the size limit specified.
    public static final int WINDOW_WIDTH = 600;
    public static final int WINDOW_HEIGHT = 400;

    //Creates a Square object Array
    Square[] squareArray = new Square[20];


    public Game() {

        //initializes square objects
        for (int i = 0; i < squareArray.length; i++)
            squareArray[i] = new Square();
    }

    public void paint(Graphics graphics) {

        //makes background black
        graphics.setColor(Color.black);
        graphics.fillRect(0, 0, WINDOW_WIDTH, WINDOW_HEIGHT);

        //paints square objects to the screen
        for (Square aSquareArray : squareArray) {
            aSquareArray.paint(graphics);
        }
    }

    public void update() {

        //calls the Square class update method on the square objects
        for (Square aSquareArray : squareArray) aSquareArray.update();
    }

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

        Game game = new Game();
        JFrame frame = new JFrame();
        frame.add(game);
        frame.setVisible(true);
        frame.setSize(WINDOW_WIDTH, WINDOW_HEIGHT);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setTitle("Raining Multiple Squares");
        frame.setResizable(false);
        frame.setLocationRelativeTo(null);

        while (true) {
            game.update();
            game.repaint();
            Thread.sleep(10);
        }
    }
}

Square class:

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

public class Square extends JPanel {

    private int squareXLocation;
    private int squareSize;
    private int squareYLocation = -squareSize;
    private int fallSpeed = 1;
    Random rand = new Random();

    /*
    //creates a random value inside the window and stores it in squareXLocation
    */
    public int generateRandomXLocation(){
        return squareXLocation = rand.nextInt(Game.WINDOW_WIDTH - squareSize);
    }

    /*
    //creates a random value between 1-50 and stores it in squareWidth
    */
    public int generateRandomSquareSize(){
        return squareSize = rand.nextInt(50);
    }

    /*
    //creates a random value that is not zero and stores it in fallSpeed(so squares do not get stuck at 0 speed)
    */
    public int generateRandomFallSpeed(){
        return fallSpeed = rand.ints(1, 1, 10).findFirst().getAsInt();
    }

    /*
    //paints the square with the variables generated in the random methods
    */
    public void paint(Graphics g){
        g.setColor(Color.CYAN);
        g.fillRect(squareXLocation,squareYLocation,squareSize,squareSize);
    }

    /*
    //sets the squareWidth and square fallSpeed to a random value for every square created
    */
    public Square(){
        generateRandomXLocation();
        generateRandomSquareSize();
        generateRandomFallSpeed();
    }

    public void update(){

        //changes the squares xLocation and fallSpeed if the created square reaches the bottom of the screen
        if(squareYLocation >= Game.WINDOW_HEIGHT){
            generateRandomXLocation();
            generateRandomFallSpeed();
            generateRandomSquareSize();
            squareYLocation = -squareSize;
        }

        //moves the square down if the square is inside the window
        if(squareYLocation <= Game.WINDOW_HEIGHT){
            squareYLocation += fallSpeed;
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Well done, you're making good progress.

Structural changes:

You should not be reusing the squares. Reusing objects is more efficient, and is great for things like particle systems (where tons of particles are created and destroyed every second). But your program is nowhere near resource intensive enough that reusing objects is necessary.

Reusing objects is unclear. It's not always easy to reset an object to it's original state and is not always clear when an object is reset.

You should create a new square whenever you need one, and remove them from your list when they reach the bottom.

Below is a review of the code as it currently is. Some of it will be obsolete when you stop reusing squares, so keep that in mind.

The game class:

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

Be careful with importing .* you're importing a lot of stuff you don't need, this makes reading the code harder.

When reading code you're always wondering about the intent of the author. "Did they really mean to use a string there", and other questions like them. So as an author, you have to try your hardest to be clear about your intent. This involves not importing everything, but only the things you need.

As a side note, I talk a lot about the reader, and other people reading your code, which you might not be too worried about if you're working alone. In that case please remember that you will have to read your code as well. If you come back to your code in six months, it will be like you've never seen it before. So please make it easy on yourself, and show your intent as clearly as you can.

public class Game extends JPanel {

Game is an ok name, but you could be a bit more precise, what kind of game is it?

    //changing these values will change the size of the game, while still remaining functional
    //within the size limit specified.
    public static final int WINDOW_WIDTH = 600;
    public static final int WINDOW_HEIGHT = 400;

    //Creates a Square object Array
    Square[] squareArray = new Square[20];

While arrays are the most efficient, in this case I think it's better to use a list instead. Using a list will make it much easier to add and remove squares.

The 20 you use in the array can be a constant (TOTAL_AMOUNT_OF_SQUARES).

    public Game() {

        //initializes square objects
        for (int i = 0; i < squareArray.length; i++)
            squareArray[i] = new Square();
    }

I noticed that every method in this class starts with an empty line. This is not necessary and looks a bit sloppy.

    public void paint(Graphics graphics) {

        //makes background black

This comment is not really nessesary. The code it describes is self explanatory. It can create some annoying mistakes, because it is easy to forget to change the comment when you change the code. Here is some nice discussion on when to use comments. Be sure to look around the related questions as well.

        graphics.setColor(Color.black);

The background can be made into a constant as well (BACKGROUND_COLOR).

        graphics.fillRect(0, 0, WINDOW_WIDTH, WINDOW_HEIGHT);

        //paints square objects to the screen
        for (Square aSquareArray : squareArray) {
            aSquareArray.paint(graphics);
        }

The name of the square in the loop does not have to be aSquareArray, but can simply be square. so the line can read "for (Square square: squareArray)".

    }

    public void update() {

        //calls the Square class update method on the square objects
        for (Square aSquareArray : squareArray) aSquareArray.update();
    }

Always use brackets for loops. I know, I know, they take up a lot of space, but if you don't use them, you will run into some nasty problems. (to bracket or not to bracket, has by this point turned into somewhat of a holy war, but especially for beginners I recommend using them, because even though they take up space, they promote clarity).

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

        Game game = new Game();
        JFrame frame = new JFrame();
        frame.add(game);
        frame.setVisible(true);
        frame.setSize(WINDOW_WIDTH, WINDOW_HEIGHT);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setTitle("Raining Multiple Squares");
        frame.setResizable(false);
        frame.setLocationRelativeTo(null);

This whole frame setup is pretty long, you could consider moving it to it's own method, just to keep it out of the way.

        while (true) {
            game.update();
            game.repaint();
            Thread.sleep(10);
        }
    }
}

The Square class:

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

public class Square extends JPanel {

Square does not have to extend JPanel, and actually it shouldn't. Because it's not a JPanel.

    private int squareXLocation;
    private int squareSize;
    private int squareYLocation = -squareSize;

The square size is still 0 at this point, so you're setting the location at 0 again.

    private int fallSpeed = 1;

Setting fallspeed is not necessary, since you're changing it later anyways.

    Random rand = new Random();

    /*
    //creates a random value inside the window and stores it in squareXLocation
    */
    public int generateRandomXLocation(){
        return squareXLocation = rand.nextInt(Game.WINDOW_WIDTH - squareSize);
    }

This method returns an int that isn't used anywhere. Either use that int to set the location, or don't return anything.

The generate methods should not be public. They should only be used by the square, and not outside of it. Make them private. Always make methods as private as possible.

    /*
    //creates a random value between 1-50 and stores it in squareWidth
    */
    public int generateRandomSquareSize(){
        return squareSize = rand.nextInt(50);
    }

    /*
    //creates a random value that is not zero and stores it in fallSpeed(so squares do not get stuck at 0 speed)
    */
    public int generateRandomFallSpeed(){
        return fallSpeed = rand.ints(1, 1, 10).findFirst().getAsInt();
    }

You can replace rand.ints(1, 1, 10).findFirst().getAsInt() with rand.nextInt(9)+1; to simplify.

    /*
    //paints the square with the variables generated in the random methods
    */
    public void paint(Graphics g){
        g.setColor(Color.CYAN);
        g.fillRect(squareXLocation,squareYLocation,squareSize,squareSize);
    }

The color can be made into a constant or a variable (SQUARE_COLOR) to allow for easier changing later on.

    /*
    //sets the squareWidth and square fallSpeed to a random value for every square created
    */
    public Square(){
        generateRandomXLocation();
        generateRandomSquareSize();
        generateRandomFallSpeed();
    }

You're forgetting to reset the y location at this point. You'll also notice that this code is exactly the same as the code in the if(squareYLocation >= Game.WINDOW_HEIGHT){ block. This is a good moment to abstract this code. You can create a "reset()" method here and call all the generate code from there. Then you can reuse the reset method in both places.

    public void update(){

        //changes the squares xLocation and fallSpeed if the created square reaches the bottom of the screen
        if(squareYLocation >= Game.WINDOW_HEIGHT){
            generateRandomXLocation();
            generateRandomFallSpeed();
            generateRandomSquareSize();
            squareYLocation = -squareSize;
        }

        //moves the square down if the square is inside the window
        if(squareYLocation <= Game.WINDOW_HEIGHT){
            squareYLocation += fallSpeed;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Yet again thanks for the review! Your code reviews are very helpful and I am learning lots from them. \$\endgroup\$ – Krzysztof Dec 23 '14 at 20:25
3
\$\begingroup\$

Only Game class reviewed

import javax.swing.*;

Better don't use star imports as one day something may get imported what you don't want. Let your IDE fix it.

//changing these values will change the size of the game, while still remaining functional

There should be a space after "//".

I read it twice, but I'm stil unsure about what it practically means. If the change is planed, then drop the constants and use fields, possibly mutable, so the change is possible while the program is running.

public void paint(Graphics graphics) {

IIRC you should override paintComponent instead of paint.

    //makes background black
    graphics.setColor(Color.black);

... while I thought that Color.black stands for pink. The uppercase color constants are preferred.

   graphics.fillRect(0, 0, WINDOW_WIDTH, WINDOW_HEIGHT);

No. You want to fill the whole panel, so use getWidth() and getHeight().

    for (int i = 0; i < squareArray.length; i++)
        squareArray[i] = new Square();

Ever heard of "goto-fail"? This is risky. The general wisdom is to put braces everywhere (I personally disagree and use one-liners, but never such two-liners).

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

Throwing exceptions from main is frowned upon (I personally find it perfectly OK for tools; but a game would deserve better). If someone starts the game by clicking, they don't get to see the exception trace.

frame.setVisible(true);

After this line, the frame is taken over by the AWT thread and must not be manipulated directly from other threads. Make this line last or use SwingUtilities.invokeLater.

    while (true) {
        game.update();
        game.repaint();
        Thread.sleep(10);
    }

The problem with this is that the total duration is unknown. It's 10 milliseconds plus something. You should use a timer or compute the sleep interval based on the elapsed time.

\$\endgroup\$
  • \$\begingroup\$ Reading your post I can't tell if it is half sarcasm or what. Why should i have a space after //? you thought black is pink? why should I use getWidth() over just using the WINDOW_WIDTH? Throwing an exception does not produce a stack trace? Also I stated above that I did not implement FPS or UPS, I was just working on the concept of making multiple objects fall, hence Thread.sleep not having an exact value. I forgot to put braces around my foreach loops so thank you for pointing that out. \$\endgroup\$ – Krzysztof Dec 17 '14 at 19:19
  • \$\begingroup\$ @Krzysztof Sorry, some sarcasm, I hope not too much. Przepraszam za konfuzje. The space after "//" is probably stated in the conventions, at least that's the way I know it. Black is pink - this one was sarcastic about the usefulness of the comment. getWidth will work even when it changes (this is more important for top level windows, which can resized from the outside). When a program get started with the mouse, there's no console for the exception to print on. So it may be better to pop up a window. Maybe better, no way necessary. \$\endgroup\$ – maaartinus Dec 17 '14 at 19:54
  • \$\begingroup\$ @Krzysztof I'm not saying that using Thread.sleep this way is wrong. If it does, what you want, fine. I'm just pointing to all (potential) problems I can see. It's a CR, it's not meant to make you down. Take what helps (even when it were just the braces), leave what doesn't. \$\endgroup\$ – maaartinus Dec 17 '14 at 19:57
  • \$\begingroup\$ Apart from black==pink, everything was meant seriously. Some sarcasm is my way to get more motivated. I recall having a lot of fun reading and writing CRs "in revenge" with a guy having exactly my sense of humor. The length of my review doesn't mean the code is wrong - a lot of minor things, but no serious problem. \$\endgroup\$ – maaartinus Dec 17 '14 at 20:00
2
\$\begingroup\$

You are returning squareSize, squareXLocation and fallSpeed from the methods where they are set but the return values are not used.

For example, you have:

public int generateRandomSquareSize(){
    return squareSize = rand.nextInt(50);
}

I'm not sure why you wouldn't just use

public void generateRandomSquareSize(){
    squareSize = rand.nextInt(50);
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for pointing this out. I am using fallSpeed in the update method for square, is the value created from generateFallSpeed() not being used? I realize that I do not need to return an int for squareXLocation or squareSize though. \$\endgroup\$ – Krzysztof Dec 17 '14 at 19:23
  • \$\begingroup\$ you are using the class' private value for fallSpeed in the update method for Square, not the one returned from generateFallSpeed(). So, you don't need to both set it and return it. \$\endgroup\$ – mcgyver5 Dec 17 '14 at 19:45

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.