Ball Entity for a Entity/Component/System Soccer Game

Question

Here is a code snippet of a class that I be interested to refactor. I was in doubt on how to proceed with the process of instantiation. The original ask was here. After think about it and heard some opinions I end up refactoring and splitting the instantiation in several constructors, one public and all others private. what do you think of this approach?

I take this opportunity to ask you to evaluate other aspects of the code.

public class BallEntity extends UserEntity {
    public static final float SCALE_FACTOR = 0.78f;
    public static final String BALL = "ball";
    private Body ballBody;
    private Camera camera;
    private Sprite ball;
    private Fixture ballFixture;

    public BallEntity(TextureAtlas atlas, RubeSceneHelper rubeSceneHelper, Camera camera) {
        this(ScaledSprite.createUsingHeight(new Sprite(atlas.findRegion(BALL)), SCALE_FACTOR), rubeSceneHelper);
        this.camera = camera;
    }

    private BallEntity(ScaledSprite scaledSprite, RubeSceneHelper rubeSceneHelper) {
        this(scaledSprite.getSprite(), rubeSceneHelper.getBody(BALL), rubeSceneHelper);
    }

    private BallEntity(Sprite sprite, Body ballBody, RubeSceneHelper rubeSceneHelper) {
        this(rubeSceneHelper.getFixture(ballBody, BALL));
        this.ball = sprite;
        this.ballBody = ballBody;
    }

    private BallEntity(Fixture ballFixture) {
        this.ballFixture = ballFixture;
    }

    @Override
    public Array<Component> getComponents() {
        Array<Component> components = new Array<Component>();
        components.add(PositionComponent.newInstance());
        components.add(CameraFollowerComponent.newInstance(camera));
        components.add(SpriteComponent.newInstance(ball));
        components.add(BodyComponent.newInstance(ballBody));
        components.add(BallContextComponent.newInstance());
        return components;
    }

    @Override
    public void init(Entity entity) {
        BodyComponent bodyComponent = entity.getComponent(BodyComponent.class);
        bodyComponent.setPosition(Vector2.Zero);
        ballFixture.setUserData(new FixtureUserData(FixtureType.BALL, entity));
    }
}



public abstract class UserEntity {

    private final Entity entity;
    private boolean wasBuilt = false;
    private Array<Class<? extends Component>> componentClasses;

    protected UserEntity() {
        this.entity = new Entity();
    }

    public final Entity getEntity() {
        if (!wasBuilt) {
            final Array<Component> components = getComponents();
            componentClasses = new Array<Class<? extends Component>>();
            for (Component c : components) {
                entity.add(c);
                componentClasses.add(c.getClass());
            }
            init(entity);
        }
        return entity;
    }

    protected abstract Array<Component> getComponents();

    public Class<? extends Component>[] getComponentClasses() {
        if (!wasBuilt) {
            throw new IllegalArgumentException("It needs to build entity before use it.");
        }
        return componentClasses.toArray();
    }

    /* Override when need to init some components */
    public void init(Entity entity) {

    }

    public void addToEngine (Engine engine){
        engine.addEntity(entity);
    }

}

Answer 1

Looking at the class from the outside, this is how it gets created:

public BallEntity(TextureAtlas atlas, RubeSceneHelper rubeSceneHelper, Camera camera)

How the class actually gets created requires jumping between a sequence of private constructors... that isn't needed at all.

public BallEntity(TextureAtlas atlas, RubeSceneHelper rubeSceneHelper, Camera camera) {
    this.camera = camera;
    this.ballFixture = rubeSceneHelper.getFixture(ballBody, "ball");

    Sprite sprite = new Sprite(atlas.findRegion(BALL))
    this.ball = ScaledSprite.createUsingHeight(sprite, SCALE_FACTOR).getSprite();
    this.ballBody = rubeSceneHelper.getBody("ball");
}

Here I've introduced an intermediate Sprite object variable, only because all those chained calls wouldn't look right.

Isn't it easier to follow? :)

---

This seems pretty verbose:

public Array<Component> getComponents() {
    Array<Component> components = new Array<Component>();
    components.add(PositionComponent.newInstance());
    components.add(CameraFollowerComponent.newInstance(camera));
    components.add(SpriteComponent.newInstance(ball));
    components.add(BodyComponent.newInstance(ballBody));
    components.add(BallContextComponent.newInstance());
    return components;
}

I'm more into C#, but I'm pretty sure (make it 51%) you could do this instead (assuming Array<T> is intended to work like a T[]):

public Component[] getComponents() {
    return new Component[] {
        PositionComponent.newInstance(),
        CameraFollowerComponent.newInstance(camera),
        SpriteComponent.newInstance(ball),
        BodyComponent.newInstance(ballBody),
        BallContextComponent.newInstance()
    };
}

Answer 2

public BallEntity(TextureAtlas atlas, RubeSceneHelper rubeSceneHelper, Camera camera) {
    this(ScaledSprite.createUsingHeight(new Sprite(atlas.findRegion(BALL)), SCALE_FACTOR), rubeSceneHelper);

I'm not a fan of creating new objects in constructors. The Sprite is a dependency and dependencies should be created outside of the class and injected in through the constructor. However, it looks like the creation of the dependencies is pretty complicated, so I do see why you chose to consolidate it in one place. The constructor just isn't the place to do it. Consider making yourself a factory class or using an inversion of control container.