3
\$\begingroup\$

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);
    }

}
\$\endgroup\$
4
\$\begingroup\$

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()
    };
}
\$\endgroup\$
  • 1
    \$\begingroup\$ With array initializers, you can even put a comma after the last element, which makes rearranging them much easier. \$\endgroup\$ – maaartinus Jun 24 '15 at 23:06
  • \$\begingroup\$ You are right, I like this kind o syntax. I don't remember why I didn't do like getComponentClasses(). But I will change. \$\endgroup\$ – alexpfx Jun 25 '15 at 23:01
4
\$\begingroup\$
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.

\$\endgroup\$

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.