Classes for a small space invaders game

Question

Below are some of the classes I have written for a small space invaders game. It's not finished but it's at the bare bones stage. I am still learning how to properly use the Slick2D library. I know where there are some problems but that is due to my lack of full knowledge of how Slick2D works, so I had to think of a workaround for the time being.

I have only created this code so you can all check it out and tell me where I am going wrong and what I am doing right. I'd also like some feedback on the code in general, such as if it is clean, if it is organized, if the naming conventions for methods and variables helpful, and other things like that. I don't know if I am a good programmer or not so hopefully this may shine some light on the situation.

Entity class

package com.emeraldzonegames.jinvaders.entities;

import java.awt.Rectangle;

import org.newdawn.slick.GameContainer;
import org.newdawn.slick.Image;
import org.newdawn.slick.Graphics;

public abstract class Entity 
{
public Image entityImage;
public int x, y, width, height,speed;
public Rectangle entityRect;

/**
 * Method that loads an image for each entity
 * @param imageName
 */
public void loadImage(String imageName)
{
    try
    {
        entityImage = new Image("Assets/Graphics/"+imageName+".png");
    }
    catch(Exception e)
    {
        e.printStackTrace();
    }
}

//Getters and setters
public void setPosition(int x, int y)
{
    this.x = x;
    this.y = y;
}

public void setDimenseions(int width, int height)
{
    this.width = width;
    this.height = height;
}

public void setSpeed(int speed)
{
    this.speed = speed;
}

public int getX()
{
    return x;
}

public int getY()
{
    return y;
}

public int getWidth()
{
    return width;
}

public int getHeight()
{
    return height;
}

public int getSpeed()
{
    return speed;
}

public Image getImage()
{
    return entityImage;
}

/**
 * Creates a rectangle for the entity
 */
public void createRectangle()
{
    entityRect = new Rectangle(getX(),getY(), getWidth(), getHeight());
}

/**
 * Method to carry out logic if input is required
 * @param gc
 * @param deltaTime
 */
public void entityLogic(GameContainer gc,int deltaTime){}

/**
 * Method to render objects to the screen
 * @param g
 */
public void renderEntity(Graphics g){}

}

Player class.

package com.emeraldzonegames.jinvaders.entities;    

import java.util.ArrayList;
import org.newdawn.slick.Input;
import org.newdawn.slick.Graphics;
import org.newdawn.slick.GameContainer;

/**
 * 
 * ClassName: Player.java
 * -------------------------------------
 * Description: The purpose of this class is to
 * Define all of the operations that are carried
 * out by the Player Object
 * 
 */

public class Player extends Entity 
{
   public ArrayList<Bullet> bulletList;

   private boolean firing;
   private final String ENTITY_ID = "ship";


   public Player()
   {    
       bulletList = new ArrayList<Bullet>();
       this.setImage(ENTITY_ID);
       this.setPosition(350,450);
       this.setDimenseions(100, 100);
       this.setSpeed(5);
       this.createRectangle();
   }

   @Override
   public void entityLogic(GameContainer gc, int deltaTime) 
   {
       Input input = gc.getInput();

       if(input.isKeyDown(Input.KEY_A))
       {
           this.x -= speed;
           this.entityRect.x -= speed;
       }

       if(input.isKeyDown(Input.KEY_D))
       {
           this.x += speed;
           this.entityRect.x -= speed;
       }

       if(input.isKeyDown(Input.KEY_W))
       {
           this.y -= speed;
           this.entityRect.y -= speed;
       }

       if(input.isKeyDown(Input.KEY_S))
       {
           this.y += speed;
           this.entityRect.y += speed;
       }

       if(input.isKeyPressed(Input.KEY_SPACE))
       {
           firing = true;
           addToList();
       }

       checkBounds();
       fireBullet();
       destroyBullets();
   }

   @Override
   public void renderEntity(Graphics g) 
   {
       g.drawImage(this.getImage(), this.getX(), this.getY());

       if(firing)
       {
           for(Bullet b : bulletList)
           {
               b.renderEntity(g);
           }
       }

       if(com.emeraldzonegames.jinvaders.main.JInvadersGame.debugModeEnabled)
       {
           g.drawRect(x, y, width, height);
       }
   }


      /**
    * Populates the arrayList with instances of the bullet object
    */
   private void addToList()
   {
       Bullet bullet = new Bullet(this.x+40,this.y);
       bulletList.add(bullet);
   }

   /**
    * Moves the bullet when player is firing
    */
   private void fireBullet()
   {
       if(firing)
       {
           for(Bullet b:bulletList)
           {
               b.moveBullet();
           }
      }
   }

   /**
    * Destroys intances of the bullet once the bullet goes out of it's bounds
    */
   private void destroyBullets()
   {
       for(int i = 0; i < bulletList.size(); ++i)
       {
           Bullet bullet = bulletList.get(i);

           if(bullet.y <= 0)
           {
               bulletList.remove(i);
               i--;
           }
       }
   }

   /**
    * Method to check if the player is on the screen at all times
    */
   private void checkBounds()
   {
       if( this.x > 720)
       {
           this.x = 720;
       }

       if( this.x < 0)
       {
           this.x = 0;
       }

       if( this.y > 450)
       {
           this.y = 450;
       }
   }
 }

Bullet

package com.emeraldzonegames.jinvaders.entities;

import org.newdawn.slick.GameContainer;
import org.newdawn.slick.Graphics;


public class Bullet extends Entity
{
private final String ENTITY_ID = "bullet";

public Bullet(int x , int y)
{
    this.x = x;
    this.y = y;
    this.loadImage(ENTITY_ID);
    this.setDimenseions(15, 15);
    this.createRectangle();
}

/**
 * Moves the bullet up the Y-Axis
 */
public void moveBullet()
{
    this.setSpeed(3);
    this.y -= speed;
    this.entityRect.y -= speed;
}

@Override
public void entityLogic(GameContainer gc, int deltaTime){}

@Override
public void renderEntity(Graphics g) 
{
    g.drawImage(this.getImage(), this.getX(), this.getY());

    if(com.emeraldzonegames.jinvaders.main.JInvadersGame.debugModeEnabled)
    {
        g.drawRect(x, y, width, height);
    }
}
}

Enemy

package com.emeraldzonegames.jinvaders.entities;


import org.newdawn.slick.GameContainer;
import org.newdawn.slick.Graphics;

public class Enemy extends Entity 
{
private final String ENTITY_ID = "testEnemy";
private final int CREATED = 1;
private final int HIT_LEFT_WALL = 2;
private final int HIT_RIGHT_WALL = 3;

public int currentState;

public Enemy(int x, int y)
{
    this.x = x;
    this.y = y;
    this.speed = 3;
    this.currentState = CREATED;
    this.setDimenseions(40,35);
    this.loadImage(ENTITY_ID);
    this.createRectangle();
}

@Override
public void entityLogic(GameContainer gc, int deltaTime) 
{
    //TODO add enemy logic

    /*if(this.x < 0)
    {
        currentState = HIT_LEFT_WALL;
        moveDown();
    }

    if(this.x > 720)
    {
        currentState = HIT_RIGHT_WALL;
        moveDown();
    }

    switch(currentState)
    {
        case 1: moveLeft(); 
        break;

        case 2: moveRight();
        break;

        case 3: moveLeft();
        break;
    }*/
}

/*private void moveLeft()
{
    this.x -= speed;
    this.entityRect.x -= speed;
}

private void moveRight()
{
    this.x += speed;
    this.entityRect.x += speed;
}

private void moveDown()
{
    this.y = y+10;
    this.entityRect.y = entityRect.y+10;
}*/

@Override
public void renderEntity(Graphics g) 
{
    g.drawImage(this.getImage(), x, y);

    if(com.emeraldzonegames.jinvaders.main.JInvadersGame.debugModeEnabled)
    {
        g.drawRect(x, y, width, height);
    }
}
}

Main Game Class

package com.emeraldzonegames.jinvaders.main;

import java.util.ArrayList;
import org.newdawn.slick.Input;
import org.newdawn.slick.Music;
import org.newdawn.slick.Image;
import org.newdawn.slick.Graphics;
import org.newdawn.slick.BasicGame;
import org.newdawn.slick.GameContainer;
import org.newdawn.slick.SlickException;
import org.newdawn.slick.AppGameContainer;

import com.emeraldzonegames.jinvaders.entities.Player;
import com.emeraldzonegames.jinvaders.entities.Enemy;
import com.emeraldzonegames.jinvaders.entities.Bullet;

/**
 *
 * ClassName: JInvadersGame
 * -------------------------------------
 * Description: The purpose of this class is to
 * create all of the assets and display them on screen
 * carry out logic if required
 * 
 */
public class JInvadersGame extends BasicGame
{
   private Player player;
   private ArrayList<Enemy> enemyList;
   private Music backgroundSong;
   private Image background;

   public static boolean debugModeEnabled = false;

   public JInvadersGame(String title)
   {
       super(title);
   }

   /**
    * Slick2D Method initialise components
    */
   @Override
   public void init(GameContainer gc) throws SlickException
   {
       player = new Player();
       enemyList = new ArrayList<Enemy>();
       backgroundSong = new Music("Assets/Audio/DST-2ndBallad.ogg");
       background = new Image("Assets/Graphics/background.jpg");
       backgroundSong.loop();

       //Creates the enemies
       createEnemies(10,3);
   }

   /**
    * Slick2D Method rendering
    */
   @Override
   public void render(GameContainer gc, Graphics g) throws SlickException
   {
       g.drawImage(background, 0, 0);
       player.renderEntity(g);

       for(Enemy e : enemyList)
       {
           e.renderEntity(g);
       }
   }

   /**
    * Slick2D Method game loop
    */
   @Override 
   public void update(GameContainer gc, int deltaTime) throws SlickException
   {

       if(debugModeEnabled == false && gc.getInput().isKeyPressed(Input.KEY_GRAVE))
       {
           debugModeEnabled = true;
       }

       else if(debugModeEnabled == true && gc.getInput().isKeyPressed(Input.KEY_GRAVE))
       {
           debugModeEnabled = false;
       }


       player.entityLogic(gc,deltaTime);

       for(Enemy e : enemyList)
       {
           e.entityLogic(gc, deltaTime);
       }

       checkCollisions();  
   }

   /**
    * Creates a grid of enemies
    */
   private void createEnemies(int row, int col)
   {
       for(int i = 1; i <= row; i++)
       {
           for(int j = 1; j <= col; j++)
           {
               Enemy enemy = new Enemy(i*50, j*50);
               enemyList.add(enemy);
           }
       }

       System.out.println(enemyList.size());
   }

   /**
    * Check for collisions
    */
   private void checkCollisions()
   {
       //Get the rectangles from each bullet and enemy from the respective ArralyLists
       for(int i = 0; i < player.bulletList.size(); ++i)
       {
           Bullet b = player.bulletList.get(i);

           for(int j = 0; j < enemyList.size(); ++j)
           {
               Enemy e = enemyList.get(j);

               //If a bullets rectangle connects with an enemies remove both objects from their lists
               if(b.entityRect.intersects(e.entityRect))
               {
                   player.bulletList.remove(i);
                   enemyList.remove(j);
                   i--;
                   j--;
               }
           }
       }
   }

   public static void main(String[] args) 
{
    try
    {
        AppGameContainer jinvaders = new AppGameContainer(new JInvadersGame("EmeraldZoneGames - JInvaders"));
        jinvaders.setDisplayMode(800, 600, false);
        jinvaders.setVSync(true);
        jinvaders.setShowFPS(false);
        jinvaders.start();
    }
    catch(SlickException se)
    {
        se.printStackTrace();
    }

}
}

Answer 1

Caveat: I am not a Java programmer. I am an ActionScript programmer. The two languages are somewhat related, and basic design principles should carry across.

At first glance, I don't see anything that would scare me away from hiring you if it were my decision. I would say that most of this code is better/cleaner than code I have seen from Computer Science graduates who have been programming 10 or more years.

So, what follows are just some observations that I have after 15 years or so of developing in AS + a few other languages.

1. Your Entity looks to me like an Abstract Class. Doesn't Java have formal support for these? You may or may not get brownie points for making this an Abstract Class.
2. setDimenseions--typo. Your IDE is clearly helping you here, but if you are using this for portfolio code, why make mistakes you don't need to make?
3. Your entityLogic method is getting information it doesn't need. Your Player Entity is the only one where you've fully fleshed out and used this method, and all you need there is the input, not the full GameContainer. If you need the full GameContainer, why not store it, rather than pushing it in every loop? I think at some level you realize how bad this is, since you're completely ignoring the method on Bullet. Yes, I know you built this off Slick2D's example code, but if Slick2D jumped off a cliff, would you?
4. Having renderEntity on Entity violates Single Responsibility Principle IMO. This Class reads more to me like a Value Object than a View Object, so it should not be performing View type activities. Again, you've followed less than stellar example code. Instead, just have your Main Class do something like g.drawImage(e.getImage(), e.getX(), e.getY()), or you could even have an EntityGraphics Class that can take an entity and draw it. Either of these solutions handily eliminates the unneccessary dependency both on Graphics and that horrible static debugMode throughout the Entity inheritance chain.
5. In Entity, sometimes you refer internally to the private storage variables, and sometimes you call the "getters." Why?
6. If you can't come up with a more descriptive JavaDoc comment for Player than what you used, you're probably better off leaving it off. We all know what Classes are for.
7. I'd go with a constructor argument for the image name, rather than a static constant in your Entity SubClasses. What if you want a Player that's a submarine? Consider this idea for the other values you're coming up with "out of thin air" in the constructor.
8. As I said, I see your Entity more as a VO, so Player shouldn't be responsible for creating its own bullets. What if you wanted to have more than one Player, with a global Object Pool of Bullets that recycles when they are taken off screen? I think you need to think a bit more about who should be responsible for what, where the Bullets are concerned. You have a mish-mash where some of it is handled by the Player and some of it is handled by the Main game, yet none of it goes through the "official" channel Entity should use for updating itself, entityLogic. Since your Main Class already concerns itself with looping through the Bullets, I'd probably relegate Player to simply storing the list of Bullets, and allow Main or some new Class to handle that.
9. Enemy--all that commented out code implies you're not a confident user of version control. Either fix it or take it out.
10. All those try/catch statements. Is there something specific that you expect could cause an error here? If so, test for that and fix it if possible (for example, null values).

Keep in mind, these are the type of suggestions I would offer to a very experienced developer, if I thought he/she was open to hearing it. I suspect that for most jobs, especially entry-level jobs, these points would be completely irrelevant. Note that if you take my suggestions that would have you deviating from the Slick2D example code, you might wind up shooting yourself in the foot with developers who feel it's better to stick to established norms for a variety of reasons, some of them good.

I think your code shows solid competence, and I wouldn't hesitate to hire you if all the other parts of the interview and hiring process lined up. Good luck!

Answer 2

First of all, all 10 points made by @Amy Blankenship is valid points as far as I can see. My comments are as follows:

1. These fields should be protected. As a rule, if a field is not final it should not be public also.

   public Image entityImage;
   public int x, y, width, height,speed;
   public Rectangle entityRect;
   

2. x, y, width, height and entityRect contain duplicate information. This duplication complicates your code elsewhere and is a probable source of bugs.

3. entityImage,x, y, width, height are dependencies of Entity. That is an Entity is not in a valid state unless all these are supplied. Therefore these should go into the constructor. Since Entity is abstract, its constructor should be protected. Something like this:

   protected Entity(Image image, Rectangle rect, int speed)
   

   As it is the code betrays your limited Java knowledge/experience.

4. Another rule of thumb: Do not repeat the name of the class in the fields. Entity.entityImage should be Entity.image and Entity.entityRect should be Entity.rect

5. setPosition(int x, int y) and setDimenseions(int width, int height) are called only from the constructors of its subclasses, therefore they are not needed once you implement protected Entity(Image image, Rectangle rect, int speed).

6. Rule of thumb: Avoid setters in your core domain objects. Their names are usually meaningless and hide some other message in you domain. For example something like

    object.setX(object.getX() -  object.sped)
   

   hides something like

    object.moveLeft()
   

   In the case of setSpeed; it is used in moveBullet each time you try to move the bullet because you forgot to supply speed in the constructor of Bullet. If you had created protected Entity(Image image, Rectangle rect, int speed) the compiler would have warned you.

7. createRectangle() is called in each of the subclass constructors, and nowhere else. It is a sign that it should go into the super class constructor. doing anything other than an assignment in constructor is suspect. In this case it is related to the duplication in x,y,width,height and entityRectangle.

8. entityLogic and renderEntity has empty method body. They could be declared abstract.

9. On Player class entityLogic method does input handling, reads and modifies the states of other classes and only implemented in Player class. These are signs that it does not belong here. It belongs to your Game class. It needs to know all the entities and more suitable place to handle input. This also removes the responsibility of bullets from Player object.

10. Rule of thumb: Method names should describe what the method does, not how it does what it does. Therefore addToList should be addBullet, even fireABullet.

11. fireBullet method name is misleading. Name is "Fire bullet" but "moving the bullets if firing" is what it does. moveBulletsIfFiring could be a better name, at least it allows one to ask "Why are the bullets are moving only when the player is firing?"

        private void fireBullet() {
            if(firing)
    

12. Repeating of g.drawImage(this.getImage(), this.getX(), this.getY()); in all the sub classes of Entity is a sign that this behavior belongs to the Entity class.

13. Hard coded constants are source of problems. They decrease the maintainability of your programs.

14. Checking for negative y coordinate is conspicuously missing in checkBounds method. Also note it does not just check. maybe something like keepPlayerWithinBounds. Name might be verbose but if you had a method like Player.keepWithin(Rectangle rect) than it would be less verbose as well as removing hard coded game are size constants from player class.

15. Loading Bullet image from file each time player fires a bullet is wrong. This is true for all your entities as doing more than field assignment in constructors is a sign something fishy is going on. You can have a look at Object Factory pattern. Or object pooling as described by @Amy Blankenship.

16. Try to use most abstract type you can get away with in declarations. Instead of ArrayList bulletList Collection bullets. This also enables you to ask yourself which implementation I should choose. In your case for example, using HashSet might allow some performance gains.

Answer 3

These are the first impression I got skimming through your code.

Entity

• Why do you have public fields?
• "Catching" an exception by only printing its stack trace is a bad practice. What happens to your code if I provide a wrong image name? I guess something bad, so please fail as early and as visibly as possible.
• I'd prefer to have a new class Position rather than having to keep the x, y pair.

Player

• replace ArrayList<Bullet> with List<Bullet> on the field definition. You do not need to know how the list is implemented. And keep that variable private too.

Enemy

• Remove the commented code. If you do not need it you should remove it. Otherwise you should keep it and fix it.

I also read the other replies you got so far and I agree with what they suggest to you.