1
\$\begingroup\$

What are the best practices to writing clean, expandable game code? I always end up getting overwhelmed with my games because the code gets very unorderly. I use OOP (java), but I still feel like my code is unorganized. I will get examples below.

The blocker class is basically like a pipe in flappy bird. It moves around and the player has to dodge it. The Ingot is like a coin, the player collects it and gets a point.

public class Blocker extends GameObject {

    private float width;

    private float gap;
    private Vector2 pos;

    private Vector3 toPosition; //X, Y, and Speed
    private Vector2 toGap; //X and speed

    private ColorAction colorAction;

    private Sprite bevel;

    private static Array<Blocker> blockers;

    private LinkedList<Vector3> movementQueue;
    private LinkedList<Vector2> gapQueue;

    private Ingot ingot;

    static {
        blockers = new Array<Blocker>();

    }

    public Blocker() {
        super();

        width = Config.blockerWidth;

        pos = new Vector2();
        this.gap = 7;
        this.pos.y = 10;

        currentFrame = new Sprite((Texture) Game.assetManager.get("white.png"));

        currentFrame.setSize(width, 10);

        colorAction = new ColorAction();
        colorAction.setColor(new Color(1, 1, 0, 1));
        colorAction.setEndColor(new Color(150f / 255f, 150f / 255f, 150f / 255f, 1));
        colorAction.setDuration(8);

        bevel = new Sprite((Texture) Game.assetManager.get("circle.png"));

        movementQueue = new LinkedList<Vector3>(); //X, Y, and Speed
        gapQueue = new LinkedList<Vector2>(); //X and Speed

        toPosition = new Vector3();
        toGap = new Vector2();

        blockers.add(this);

        int i = Tools.generateInt(0, 3);
        if (i == 2) {
            ingot = new Ingot(new Vector2(pos), Tools.generateInt((int) Ingot.minLifespan, (int) Ingot.maxLifespan));
        }
    }

    @Override
    public void act(float delta) {

        super.act(delta);

        colorAction.act(Gdx.graphics.getDeltaTime());

        updateMovement();

        while (pos.x > 360) {
            pos.x -= 360;
        }

        while (pos.x < 0) {
            pos.x += 360;
        }

        if (ingot != null) {
            if (ingot.isDead()) {
                ingot = null;
            } else {
                ingot.pos.set(this.pos.x, this.pos.y + gap / 2);
            }
        }

    }

    @Override
    public void draw(Batch batch, float parentAlpha) {
        super.draw(batch, parentAlpha);

        currentFrame.setColor(colorAction.getColor());
        //Inner
        currentFrame.setPosition(-width / 2, 0);
        currentFrame.setOrigin(width / 2, 0);
        currentFrame.setSize(width, pos.y);
        currentFrame.setRotation(pos.x);
        currentFrame.draw(batch);

        //Ball on end (bevel)
        bevel.setColor(colorAction.getColor());
        bevel.setPosition(-width / 2, pos.y - width / 2);
        bevel.setOrigin(width / 2, -pos.y + width / 2);
        bevel.setSize(width, width);
        bevel.setRotation(pos.x);
        bevel.draw(batch);

        //Outer
        currentFrame.setOrigin(width / 2, -pos.y - gap);
        currentFrame.setPosition(-width / 2, pos.y + gap);
        currentFrame.setRotation(pos.x);
        currentFrame.setSize(width, 50);
        currentFrame.draw(batch);

        //Ball on end (bevel)
        bevel.setColor(colorAction.getColor());
        bevel.setOrigin(width / 2, -pos.y - gap + width / 2);
        bevel.setPosition(-width / 2, pos.y + gap - width / 2);
        bevel.setRotation(pos.x);
        bevel.setSize(width, width);
        bevel.draw(batch);
    }

    public static void circlifyBlockers(float speed, boolean override) {

        float spacing = 360 / blockers.size;
        float counter = spacing;

        for (Blocker blocker : blockers) {
            blocker.moveTo(new Vector3(counter, 777, speed), override);
            counter += spacing;
        }

    }

    //Override stops current movement, false adds movement to queue | newPos is x, y, and speed
     //777 is predefined for telling number not to be overriden (eg you can pass in vec3(3, 777, 1) and those variables
    //will be set except for 777 which will remain what it was before
    public void moveTo(Vector3 newPos, boolean override) {
        if (override) {
            if (newPos.x != 777) {
                toPosition.x = newPos.x;
            }
            if (newPos.y != 777) {
                toPosition.y = newPos.y;
            }
            if (newPos.z != 777) {
                toPosition.z = newPos.z;
            }
        } else {
            movementQueue.add(newPos);
            if (toPosition != null) {
                if (newPos.x == 777) {
                    newPos.x = toPosition.x;
                }
                if (newPos.y == 777) {
                    newPos.y = toPosition.y;
                }
                if (newPos.z == 777) {
                    newPos.z = toPosition.z;
                }
            }
        }

        //In charge of creating Ingot
        if (ingot == null) {
            int i = Tools.generateInt(0, 3);
            if (i == 2) {
                getStage().addActor(ingot = new Ingot(new Vector2(pos.x, pos.y), Tools.generateInt((int) Ingot.minLifespan, (int) Ingot.maxLifespan)));
            }
        }

    }

    public void setGap(Vector2 newGap, boolean override) { //newGap is x and speed
        if (override) {
            toGap = newGap;
        } else {
            gapQueue.add(newGap);
        }
    }


    //------------------------Big stuff-------------------------/
    private void updateMovement() { //If hasn't reached toPosition/toGap, then this updates and moves

        if (pos.x != toPosition.x) {
            pos.x += toPosition.z * Gdx.graphics.getDeltaTime();
            if (Math.abs(pos.x - toPosition.x) <= Math.abs(toPosition.z) * Gdx.graphics.getDeltaTime() * 1.25f) {
                pos.x = toPosition.x;
            }
        } else if (pos.y == toPosition.y) {
            if (movementQueue.size() > 0) {
                toPosition = movementQueue.removeFirst();
            }
        }

        if (pos.y > toPosition.y) {
            pos.y -= Math.abs(toPosition.z) * Gdx.graphics.getDeltaTime();
            if (pos.y < toPosition.y) {
                pos.y = toPosition.y;
            }
        } else {
            pos.y += Math.abs(toPosition.z) * Gdx.graphics.getDeltaTime();
            if (pos.y > toPosition.y) {
                pos.y = toPosition.y;
            }
        }

        if (gap != toGap.x) { //Hasn't reached gap destination
            if (gap > toGap.x) {
                gap -= toGap.y * Gdx.graphics.getDeltaTime();
                if (gap < toGap.x) {
                    gap = toGap.x;
                }
            } else {
                gap += toGap.y * Gdx.graphics.getDeltaTime();
                if (gap > toGap.x) {
                    gap = toGap.x;
                }
            }
        } else {
            if (gapQueue.size() > 0) {
                toGap = gapQueue.removeFirst();
            }
        }

    }

    //-----------------------Getters-----------------------/
    public Vector2 getPos() {
        return pos;
    }

    public static Array<Blocker> getBlockers() {
        return blockers;
    }

    public Vector3 getFuturePosition() {
        return toPosition;
    }

    public boolean finishedMoving() {
        if (pos.x == toPosition.x && pos.y == toPosition.y) {
            return true;
        } else {
            return false;
        }
    }

    public boolean finishedGapMoving() {
        if (gap == toGap.x) {
            return true;
        } else {
            return false;
        }
    }

    public void setColorAction(Color fadeTo, float secondDuration) {
        Color color = colorAction.getColor();
        colorAction.reset();
        colorAction.setColor(color);
        colorAction.setEndColor(fadeTo);
        colorAction.setDuration(secondDuration);
        colorAction.restart();
    }

    public ColorAction getColorAction() {
        return colorAction;
    }

    public static boolean allBlockersFinishedMoving() {

        int i = 0;
        for (Blocker blocker : blockers) {
            if (!blocker.finishedMoving()) {
                i++;
            }
        }
        if (i == 0) {
            return true;
        } else {
            return false;
        }
    }

    public void setWidth(float newWidth) {
        this.width = newWidth;
    }
}

A second class called Ingot:

public class Ingot extends GameObject {

    private float lifeSpan;
    private float timeAlive;

    private Animation animation_current, animation, animation_spawn, animation_die;
    private float elapsedTime;

    private State state;

    private boolean dead;

    private enum State {
        SPAWN, IDLE, DIE
    }

    public Ingot(Vector2 pos, float lifespan) { //Lifespan in seconds
        super();
        state = State.SPAWN;

        setupAnim();

        setRevolve(false);

        this.pos = pos;
        this.currentFrame.setRotation(pos.x);

        this.lifeSpan = lifespan;

        dead = false;

        System.out.println("Ingot created");

    }

    @Override
    public void act(float delta) {
        if (!dead) {
            System.out.println("Ingot acting");
            super.act(delta);

            timeAlive += Gdx.graphics.getDeltaTime();
            elapsedTime += Gdx.graphics.getDeltaTime();

            if (timeAlive >= lifeSpan) {
                state = State.DIE;
            }

            switch (state) {
                case SPAWN:
                    if (animation_current != animation_spawn) {
                        animation_current = animation_spawn;
                        elapsedTime = 0;
                    }
                    currentFrame.setRegion(animation_current.getKeyFrame(elapsedTime));
                    if (animation_current.isAnimationFinished(elapsedTime)) {
                        state = State.IDLE;
                    }
                    break;
                case IDLE:
                    if (animation_current != animation) {
                        animation_current = animation;
                        elapsedTime = 0;
                    }
                    currentFrame.setRegion(animation_current.getKeyFrame(elapsedTime));
                    break;
                case DIE:
                    if (animation_current != animation_die) {
                        animation_current = animation_die;
                        elapsedTime = 0;
                    } else {
                        if (animation_current.isAnimationFinished(elapsedTime)) {
                            dead = true;
                            dispose();
                        }
                    }
                    currentFrame.setRegion(animation_current.getKeyFrame(elapsedTime));

                    break;
            }
        }

    }

    @Override
    public void draw(Batch batch, float parentAlpha) {
        if (!dead) {
            super.draw(batch, parentAlpha);

            currentFrame.draw(batch);
        }

    }

    @Override
    public void dispose() {
        super.dispose();
    }

    private void setupAnim() {

        //Default animation
        animation = new Animation(1f / 30f, splitSpriteSheet((Texture) Game.assetManager.get("Ingot.png"), 256, 256));
        animation.setPlayMode(Animation.PlayMode.LOOP);

        //Spawn animation
        animation_spawn = new Animation(1f / 30f, splitSpriteSheet((Texture) Game.assetManager.get("Ingot_spawn.png"), 256, 256));

        //Die animation
        animation_die = new Animation(1f / 120f, splitSpriteSheet((Texture) Game.assetManager.get("Ingot_die.png"), 256, 256));

        //Current Frame
        animation_current = animation_spawn;
        currentFrame = new Sprite();
        currentFrame.setRegion(animation_current.getKeyFrame(elapsedTime));
        currentFrame.setSize(1.5f, 1.5f);

    }

    private TextureRegion[] splitSpriteSheet(Texture spriteSheet, int frameWidth, int frameHeight) {
        TextureRegion[][] tmpFrames = TextureRegion.split(spriteSheet, frameWidth, frameHeight);
        TextureRegion[] animationFrames = new TextureRegion[tmpFrames.length * tmpFrames[0].length];
        int counter = 0;
        for (int x = 0; x < tmpFrames.length; x++) {
            for (int y = 0; y < tmpFrames[0].length; y++) {
                animationFrames[counter] = tmpFrames[x][y];
                counter++;
            }
        }

        return animationFrames;
    }

    public boolean isDead() {
        return dead;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Done, hope that was enough info. If possible, it would be nice for someone to also give some examples of separating parts of my code into separate classes. I feel like it all should be in the class it's in, but I'm no professional. \$\endgroup\$ – Wyatt Aug 5 '16 at 16:15
2
\$\begingroup\$

First things first, taken in separate parts the code is fairly easy to read. As any software system grows, the complexity increases naturally. In that sense, it is not the responsibility of the developer to manage the complexity of the system as it were (because the complexity is driven by business need / requirements). It is the responsibility of the developer to manage the flexibility of the system so that unnecessary complexity is avoided.

That being said, it seems like your problems lie with your use (or lack) of abstraction and encapsulation and your approach to the development process.

I would suggest first altering your thought processes before putting fingers to keyboard.

Questions to ask yourself when designing


  • What are the various responsibilities of the system I'm designing?
  • How are those responsibilities likely to evolve over time?
  • How modular does the system need to be to accommodate the likely changes?
  • Which organization structure allows for an optimal balance of speed and maintainability / flexibility?
  • How will each feature be tested and how do inter-dependent parts of the system need to interact with one another?
  • How would these things exist and relate in the "real world"?

Appraoch


Define Responsibilities / System Attributes

  • Players interact with objects.
  • Objects are 3D and static unless acted upon.
  • Objects should maintain their own state.
  • Positioning is not part of object state, but environment state.
  • Positioning can be affected by player interaction and object interaction.
  • Visual elements are affected by changes in state.
  • Inconsistencies between visual state and object state are possible and should be accounted for (object goes out of screen, timing delays, etc...).
  • Animations are a function of visual elements.
  • Not all objects are visual.

Once you have some of those things laid out, you can begin to plan the types you will need and how they will interact / what OO patterns accomplish the system goals / behavioral definitions.

Here are some types I would create to help separate out the code you have listed in accordance with the above assumptions:

I have created a GitHub Gist that lays out the various classes I came up with (still incomplete, but enough to demonstrate the application of the above ideas).

In the interest of full-disclosure, I haven't written Java in years so I used a converter from C#. Please treat the code in the gist as pseudo-code, not as comilable source-code. The goal of my example is more to illustrate the relationship between objects and the separation of concern than it is to be syntactically correct.

Essentially you start to wind up with many small classes, each of which has a very explicit and general way in which it interacts with other classes.

Here are some code smells I identified looking through your code listing:

1. Invalid Object References

Your classes are too smart for their own good. A class should only be concerned with it's internal workings and not be familiar with the system in which it will be used.

For example:

bevel = new Sprite((Texture) Game.assetManager.get("circle.png"));

In this case, the Blocker class knows how to retrieve image resources from the file system. Generally speaking a code architecture should trickle-down (classes know about their dependencies and dependencies should be provided to the class at the time the class is instantiated unless lazy loading is a desired strategy).

It's also general good practice for code to not have to dig several layers deep into dependent code. Having to reference something like:

myInstance.dependency.behavior().resultData[0].itemValue() is a BIG code smell because it makes your dependency chain very deep and difficult to change. If that were all over the code, imagine the impact of changing anything in my example chain on how much time you'll need to spend unwinding the code and fixing all of the broken places.

2. Too many responsibilities

Your blocker class has the following responsibilities:

  • Define a static set of blocker instances for the application
  • Manage blocker instances
  • Draw instances of Blockers to the GUI
  • Manage Blocker positioning and hit-detection
  • Manage the lifetime of any associated ingot's
  • Animate any associated ingot
  • Parse a sprite to load texture information
  • Define the size / position of a blocker instance

At a MINIMUM, those should be 6 different classes.

The effect of a class structured like this is you need to come in here to fix / change everything.

To help illustrate the challenge, imagine you were designing a nuclear reactor.

It needs to:

  • Produce energy
  • Give off radiation
  • Be given supplies of uranium and / or plutonium
  • Have a cooling mechanism
  • Have an air-conditioning system for employees
  • Have a parking lot for employees

All of those things are related to and required for the operation of the reactor, but if you need to paint new lines on the parking lot, you would want to not have to worry about bumping the cooling mechanism for the reactor.

In the same way, the portions of your code which are unrelated to the function of the other should be separate.

  • Have a class whose responsibility it is to render components in the UI.
  • Have a class responsible for generating new instances of the Blocker and registering them with the game.
  • Have a class that loads sprite information and passes it to the UI.
  • Have a class responsible for determining if the player has collided with a blocker.

etc...

3. Do have a consistent convention for variable names.

One of the challenges of readability is your parameters, local variables, instance variables are all named the same. This makes it difficult to identify which of them is used where. Adopting a convention around this will make the code appear less "bewildering" because the meaning and scope of variables will be instantly clear.

These are just some of my thoughts and I can provide any additional clarification that you need.

\$\endgroup\$
  • \$\begingroup\$ First off, I can tell how much effort you put into this post, thank you very much for your time. Everything you explained does make sense to me, but the code on github I wasn't quite understanding. I'm still not sure how exactly to separate all of the logic I have in the blocker/ingot into separate classes. I get that I should probably pass in the sprite in the constructor, instead of loading it in the class, but I probably shouldn't create a class for each method I have in there (Not that you said that). \$\endgroup\$ – Wyatt Aug 5 '16 at 21:53
  • \$\begingroup\$ As an example, shouldn't the blocker be responsible of rendering itself? It seems odd to create a class that renders the blocker/s. \$\endgroup\$ – Wyatt Aug 5 '16 at 21:55
  • 1
    \$\begingroup\$ It's common to separate a presentation layer from the business layer. There are many ways to do it, but it would be less code duplication to have a GameObjectRenderer class that can take GameObject instances and render them to the UI based on coordinates, height, width. Then separately, you could have a class that is responsible for loading sprites. Finally a class that you build and pass to the GameObjectRenderer that contains coordinates, dimensions, sprite / color information. Things that get rendered always have those attributes so you can create any number of GameObjects in the future \$\endgroup\$ – xDaevax Aug 5 '16 at 22:04

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.