12
\$\begingroup\$

I created my first java game in LibGDX and it's working fine but I'm 100% sure a lot of my code can be written shorter than now. Does anyone have tips how I can make this code better?

Like the Gdx.input.getX(), if I run this on Android, it's fine, but when I run this on PC, you just have to hover over your screen to change the players position without clicking. On Android you have to tap first.

Does anyone have any tips how to do this better and make the game playable on pc? So you have to click to move the bar.

Here is a screenshot of my game right now.

enter image description here

Here are my 4 classes with the code I wrote:

MainScreen.java

package ***.***.***;

import com.badlogic.gdx.Gdx;
import com.badlogic.gdx.Screen;
import com.badlogic.gdx.graphics.GL20;
import com.badlogic.gdx.graphics.Texture;
import com.badlogic.gdx.graphics.g2d.BitmapFont;
import com.badlogic.gdx.graphics.g2d.SpriteBatch;
import ***.***.***.Player;
import ***.***.***.Enemy;
import ***.***.***.Ball;

public class MainScreen implements Screen {

    //GAMESTATE = 0 ---- MENU
    //GAMESTATE = 1 ---- INIT/RESET
    //GAMESTATE = 2 ---- START
    //GAMESTATE = 3 ---- UPDATE
    //GAMESTATE = 4 ---- GAMEOVER
    //GAMESTATE = 5 ---- PAUSE
    //GAMESTATE = 6 ---- EXIT
    //GAMESTATE = 100 ---- WIN

    int GAMESTATE = 0;

    int timer = 30;
    int countdown = 90;

    int score=0,lives=5;

    private Player player;
    private Enemy enemy;
    private Ball ball;
    private BitmapFont font;
    private SpriteBatch batch;
    Texture BackGround;
    Texture StartScreen;

    public static float difficulty;

    float width = Gdx.graphics.getWidth();
    float height = Gdx.graphics.getHeight();
    float screenwidth = width/270;
    float screenheight = height/480;

    public MainScreen() {
        Gdx.app.log("GameScreen", "Attached");
        player = new Player();
        enemy = new Enemy();
        ball = new Ball();

        batch = new SpriteBatch();

        //Load in Fonts
        font = new BitmapFont(Gdx.files.internal("data/whitetext.fnt"),
        Gdx.files.internal("data/whitetext.png"), false);
        BackGround = new Texture("data/background.jpg");
        StartScreen =  new Texture("data/start.jpg");
    }

    @Override
    public void render(float delta) {
        //Draw the background
        batch.begin();
        batch.draw(BackGround,0,0,270.0f*screenwidth,480.0f*screenheight);
        batch.end();

        //MENU
        if(GAMESTATE==0){
            if(timer!=0)timer--;
            if (Gdx.input.isTouched()&&timer==0) {
                GAMESTATE=1;
            }

            batch.begin();
            font.setColor(1.0f, 1.0f, 1.0f, 1.0f);
            font.setScale(1.0f*screenwidth,1.0f*screenheight);
            font.draw(batch, "PONG", 60*screenwidth, 400*screenheight);
            font.setScale(0.5f*screenwidth,0.5f*screenheight);
            font.draw(batch, "Tap to start", 20*screenwidth, 240*screenheight);
            batch.end();
        }

        //INIT
        if(GAMESTATE==1){
            player.init();
            enemy.init();
            ball.init();
            GAMESTATE=2;
            timer=30;
            difficulty=1.0f;
            countdown=90;
        }

        //START
        if(GAMESTATE==2){
            if(countdown==0){
                GAMESTATE=3;
            }
            countdown--;
            batch.begin();
            font.setColor(1.0f, 1.0f, 1.0f, 1.0f);
            font.setScale(1.0f*screenwidth,1.0f*screenheight);
            if(countdown>=60&&countdown<=90)font.draw(batch, "3", 120*screenwidth, 300*screenheight);
            if(countdown>=30&&countdown<=60)font.draw(batch, "2", 120*screenwidth, 300*screenheight);
            if(countdown>=0&&countdown<=30)font.draw(batch, "1", 120*screenwidth, 300*screenheight);
            batch.end();
        }

        //UPDATE
        if(GAMESTATE==3){
        player.update();
        enemy.update();
        ball.update();

        //DRAW SCORE/LIVES
        batch.begin();
        font.setColor(1.0f, 1.0f, 1.0f, 1.0f);
        font.setScale(0.2f*screenwidth,0.2f*screenheight);
        font.draw(batch, "Score: "+score+"/5", 190*screenwidth, 475*screenheight);
        font.draw(batch, "Lives: "+lives, 5*screenwidth, 475*screenheight);
        batch.end();
        }

        if(lives==0)GAMESTATE=4;
        if(score==5)GAMESTATE=100;

        if(ball.getY()<0*screenheight){
            lives--;
            GAMESTATE=1;
        }
        if(ball.getY()>480*screenwidth-16*screenwidth){
            score++;
            GAMESTATE=1;
        }

        //LOSE SCREEN
        if(GAMESTATE==4){
            if(timer!=0)timer--;
            if (Gdx.input.isTouched()&&timer==0) {
                GAMESTATE=1;
            }
            batch.begin();
            font.setColor(1.0f, 1.0f, 1.0f, 1.0f);
            font.setScale(0.5f*screenwidth,0.5f*screenheight);
            font.draw(batch, "Game Over", 20*screenwidth, 300*screenheight);
            font.setScale(0.3f*screenwidth,0.3f*screenheight);
            font.draw(batch, "Tap to Start", 20*screenwidth, 260*screenheight);
            batch.end();
            score=0;lives=5;
        }

        //WIN SCREEN
        if(GAMESTATE==100){
            if(timer!=0)timer--;
            if (Gdx.input.isTouched()&&timer==0) {
                GAMESTATE=1;
            }
            batch.begin();
            font.setColor(1.0f, 1.0f, 1.0f, 1.0f);
            font.setScale(0.5f*screenwidth,0.5f*screenheight);
            font.draw(batch, "You Won!", 20*screenwidth, 300*screenheight);
            font.setScale(0.3f*screenwidth,0.3f*screenheight);
            font.draw(batch, "Tap to Start", 20*screenwidth, 260*screenheight);
            batch.end();
            score=0;lives=5;
        }

        //Player Ball Colission
        if(ball.getX()+8*screenwidth>player.getX()-40*screenwidth&&ball.getX()+8*screenwidth<player.getX()+40*screenwidth){
            if(ball.getY()>player.getY()&&ball.getY()<player.getY()+16*screenheight){
                float zy;
                zy = 3*screenheight;
                ball.setZy(zy);
                difficulty+=0.1;

            }
        }

        //Enemy Ball Colission
        if(ball.getX()+8*screenwidth>enemy.getX()-40*screenwidth&&ball.getX()+8*screenwidth<enemy.getX()+40*screenwidth){
            if(ball.getY()+16*screenheight>enemy.getY()&&ball.getY()+16*screenheight<enemy.getY()+16*screenheight){
                float zy;
                zy = -3*screenheight;
                ball.setZy(zy);
                difficulty+=0.5;

            }
        }
        //Enemy AI
        if(enemy.getX()<ball.getX()){
            float ex;
            ex = 3.0f*screenwidth*difficulty;
            enemy.setZX(ex);
        }
        if(enemy.getX()>ball.getX()){
            float ex;
            ex = -3.0f*screenwidth*difficulty;
            enemy.setZX(ex);
        }
    }

    public float getDif() {
        return difficulty;
    }

    public void setDif(float difficulty) {
        this.difficulty = difficulty;
    }

    @Override
    public void resize(int width, int height) {

    }

    @Override
    public void show() {

    }

    @Override
    public void hide() {

    }

    @Override
    public void pause() {

    }

    @Override
    public void resume() {

    }

    @Override
    public void dispose() {

    }

}

Ball.java

package ***.***.***;

import java.util.Random;

import com.badlogic.gdx.Gdx;
import com.badlogic.gdx.graphics.Texture;
import com.badlogic.gdx.graphics.g2d.SpriteBatch;
import ***.***.***.MainScreen;
import ***.***.***.Player;

public class Ball {

    SpriteBatch batch;
    Texture ballsprite;

    Random random = new Random();

    float width = Gdx.graphics.getWidth();
    float height = Gdx.graphics.getHeight();
    float screenwidth = width/270;
    float screenheight = height/480;

    float x,y,zx,zy,randomx,randomw;

    float difficulty;

    private MainScreen main;

    public Ball(){
    }

    public void init(){
        batch = new SpriteBatch();
        ballsprite = new Texture("data/ball.png");

        x = 127*screenwidth;
        y = 232*screenheight;
        randomw = random.nextInt(2);
        if(randomw==1)randomw=-1;else randomw=1;
        randomx = random.nextInt(20) + 20;
        zx = randomx/10*screenwidth*randomw;
        zy = 3*screenwidth;

        main = new MainScreen();
    }

    public void update(){
        difficulty = main.getDif();
        //Colission check
        if(x<0*screenheight)zx=3*screenwidth;
        if(x>270*screenwidth-16*screenwidth)zx=-3*screenwidth;

        x += zx*difficulty;
        y += zy*difficulty;

        //Draw ball
        batch.begin();
        batch.draw(ballsprite,x,y,16*screenheight,16*screenwidth);
        batch.end();
    }

    public float getX() {
        return x;
    }

    public float getY() {
        return y;
    }

    public void setZy(float zy) {
        this.zy = zy;
    }

}

Player.java

package ***.***.***;

import com.badlogic.gdx.Gdx;
import com.badlogic.gdx.graphics.Texture;
import com.badlogic.gdx.graphics.g2d.Sprite;
import com.badlogic.gdx.graphics.g2d.SpriteBatch;

public class Player {

    float width = Gdx.graphics.getWidth();
    float height = Gdx.graphics.getHeight();
    float screenwidth = width/270;
    float screenheight = height/480;

    SpriteBatch batch;
    Texture playersprite;
    public float x,y;

    public float getX() {
        return x;
    }

    public float getY() {
        return y;
    }

    public Player(){

    }

    public void init(){
        x=135*screenwidth;
        y=40*screenheight;

        batch = new SpriteBatch();
        playersprite = new Texture("data/bar.png");
    }

    public void update(){
        x = Gdx.input.getX();

        batch.begin();
        batch.draw(playersprite,x-40*screenwidth,y,80*screenwidth,16*screenheight);
        batch.end();
    }
}

Enemy.java

package ***.***.***;

import com.badlogic.gdx.Gdx;
import com.badlogic.gdx.graphics.Texture;
import com.badlogic.gdx.graphics.g2d.SpriteBatch;

public class Enemy {

    SpriteBatch batch;
    Texture enemysprite;

    float width = Gdx.graphics.getWidth();
    float height = Gdx.graphics.getHeight();
    float screenwidth = width/270;
    float screenheight = height/480;

    public float x,y;
    public float zx;

    public Enemy(){

    }

    public void init(){
        x=135*screenwidth;
        y=444*screenheight;

        batch = new SpriteBatch();
        enemysprite = new Texture("data/bar.png");
    }

    public void update(){
        batch.begin();
        batch.draw(enemysprite,x-40*screenwidth,y,80*screenwidth,16*screenheight);
        batch.end();
        x+=zx;
        if(x>229*screenwidth)x=229*screenwidth;
        if(x<41*screenwidth)x=41*screenwidth;
    }

    public float getX() {
        return x;
    }

    public float getY(){
        return y;
    }

    public void setZX(float zx) {
        this.zx = zx;
    }
}

PongMain.java

package ***.***.***;

import com.badlogic.gdx.Game;
import com.badlogic.gdx.Gdx;
import ***.***.***.MainScreen;

public class PongMain extends Game {

    @Override
    public void create() {
        setScreen(new MainScreen());
    }
}
\$\endgroup\$
  • \$\begingroup\$ Hi, welcome to CodeReview. Hope you get some great answers to your question! \$\endgroup\$ – Abbas Jan 27 '15 at 8:27
11
\$\begingroup\$

Scope

You should reduce the scope of variables to the minimum needed. So, if possible make them private.

Something like

public float x,y;

public float getX() {
    return x;
}

public float getY() {
    return y;
}  

should be avoided because it removes encapsulation. Sometimes it is ok to expose a variable to the public, but you should usually hide them behind methods. Mixing both is a code smell.


Declaring and initializing of multiple variables should be avoided because readability matters. So instead of

int score=0,lives=5;  

it should be

private int score = 0;
private int lives = 5;  

By extracting the rendering of the different states to separate methods, your render() method will be easier to maintain and read.


Your variables would be happy to get some space to breathe. So instead of

font.setScale(0.5f*screenwidth,0.5f*screenheight);

it should look like

font.setScale(0.5f * screenwidth, 0.5f * screenheight);  

Based on the IDE you are using, there is a keyboard shortcut to format the code by using proper indention and spacing. This will add readability to your code.


Using braces {} for single if statements will make your code less errorprone and is more structured so content which belongs together comes into the focus and can be grapsed at first glance.

If you decide not to use braces you should stick to the choosen style. Right now you are mixing the usage.


Shortening of method names or variable names should be avoided for readability. If you have a method SetDif() it could mean for setting the Difference or Difficulty.


Some construct like

    //Player Ball Colission
    if(ball.getX()+8*screenwidth>player.getX()-40*screenwidth&&ball.getX()+8*screenwidth<player.getX()+40*screenwidth){
        if(ball.getY()>player.getY()&&ball.getY()<player.getY()+16*screenheight){
            float zy;
            zy = 3*screenheight;
            ball.setZy(zy);
            difficulty+=0.1;

        }
    }  

should be extracted to a separate method like hasBallCollision(Player player) which should return true if both if conditions evaluate to true. This can the be called for the enemy too which will make your code DRY (don't repeat yourself) by removing code duplication.

This would lead to something like

if (hasBallCollision(player)){
    float zy;
    zy = 3 * screenheight;
    ball.setZy(zy);
    difficulty += 0.1;
}

which could be reduced to

if (hasBallCollision(player)){
    ball.setZy(3 * screenheight);
    difficulty += 0.1;
}

You have a lot of magic numbers in your code. You should try to extract them into well named constants.


For the case that the game is won or lost, you should return early from the render() method.


Update

Unfortunately you can't use the hasBallCollision() method with the enemy and the player.

If you don't want to go with Simon André Forsberg suggestion using one class for both, which by the way is a pretty good suggestion, you could add an overloaded method which takes an Enemy as an input parameter.

You should at least simplify the if conditions. If we look closer at the first condition we see that you are adding 8 * screenwidth to the left side and substracting 40 * screenwidth from the right side

if(ball.getX() + 8 * screenwidth > player.getX() - 40 * screenwidth   
    && ball.getX() + 8 * screenwidth < player.getX() + 40 * screenwidth)  

this can be simplified to

 if(ball.getX() > player.getX() - 32 * screenwidth   
     && ball.getX() < player.getX() + 32 * screenwidth)  

or more readable

int offset = 32 * screenwidth;  

if(ball.getX() > player.getX() - offset   
     && ball.getX() < player.getX() + offset)  

For the code you had used for the enemy ball collision the second condition of the second if statement had been

    && ball.getY() + 16 * screenheight < enemy.getY() + 16 * screenheight)  

this should be simplified to

&& ball.getY() < enemy.getY() 
\$\endgroup\$
  • \$\begingroup\$ Thanks gonna try to make it more clean and readable and also the collision function is a good idea \$\endgroup\$ – Stefan Jan 27 '15 at 9:10
  • \$\begingroup\$ the player/ball collision is working but do you have any tips how i can make it work for both the enemy and the player without writing the full code again? \$\endgroup\$ – Stefan Jan 27 '15 at 9:35
  • \$\begingroup\$ Updated answer. \$\endgroup\$ – Heslacher Jan 27 '15 at 12:18
  • \$\begingroup\$ Thanks alot for the help! I'm getting alot further in my game now :-) Going to to Add a better difficulty system to it now. \$\endgroup\$ – Stefan Jan 27 '15 at 13:47
  • \$\begingroup\$ Good to hear. Feel free to post a follow up if you are ready, see this meta post. \$\endgroup\$ – Heslacher Jan 27 '15 at 13:58
10
\$\begingroup\$
public static float difficulty;

I don't really see why this is static. You only have one MainScreen instance in your game.


int GAMESTATE;

This is not a static final variable so it should not be named with ALL_CAPS. gameState is a better name.

@Heslasher is correct that you are using a lot of magic numbers in your code, especially when it comes to this variable. I think a better approach for it though is to use an enum

public enum GameState {
    MENU, INIT, START, UPDATE, GAMEOVER, PAUSE, EXIT, WIN;
}

And in your MainScreen:

private GameState gameState = GameState.MENU;

...
if (gameState == GameState.INIT) {
     ...
}

In Ball you have the name setZy, and in Player / Enemy you have setZX. Notice the capitalization difference? And what is that Z doing in that name at all? It seems like it is being used as a delta, so setDeltaX and setDeltaY would be better.

Speaking of your Player and Enemy classes, I really think those two have way too much in common to be considered different classes. Make one class and instantiate two objects of it instead.

Paddle player = new Paddle(...);
Paddle enemy  = new Paddle(...);

public class Paddle {

    private float width = Gdx.graphics.getWidth();
    private float height = Gdx.graphics.getHeight();

    private float screenwidth = width/270;
    private float screenheight = height/480;

    private SpriteBatch batch;
    private Texture sprite;

    private float x,y;
    private float zx;

    public void init(int parameter) {
        x=135*screenwidth;
        y=parameter*screenheight;

        batch = new SpriteBatch();
        playersprite = new Texture("data/bar.png");
    }

    public void update(){
        batch.begin();
        batch.draw(enemysprite,x-40*screenwidth,y,80*screenwidth,16*screenheight);
        batch.end();
        x += zx;
        if(x>229*screenwidth)x=229*screenwidth;
        if(x<41*screenwidth)x=41*screenwidth;
    }
    public void setX(int newValue) { 
        x = newValue;
    }

    public float getX() {
        return x;
    }

    public float getY(){
        return y;
    }

    public void setDeltaX(float deltaX) {
        this.deltaX = deltaX;
    }
}

Instead of player.update(); you call:

player.setX(Gdx.input.getX());
player.update();

Also make sure that you never change player.deltaX. enemy.deltaX can be changed.

\$\endgroup\$
  • \$\begingroup\$ Thanks I got it working! You are right its working fine with just one class. \$\endgroup\$ – Stefan Jan 27 '15 at 11:51

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.