Using method chains when rendering a scene

Question

Is writing Java code in chain like _hud.getStage().getCamera().combined an ugly practice?

@Override
public void render(float delta) {
    Gdx.gl.glClearColor(1,0,0,1);
    Gdx.gl.glClear(GL20.GL_COLOR_BUFFER_BIT);
    _batch.setProjectionMatrix(_hud.getStage().getCamera().combined);
    _hud.getStage().draw();
}

If yes, what would be the pretty way to write the code? And why is it ugly? What are the consequences of this?

Answer 1

Is writing Java code in chain like _hud.getStage().getCamera().combined an ugly practice?

The general design guideline (Law of Demeter) is to avoid such method chains. In some cases it can be justified.

The ugliest thing on this line is the _ prefix of a field which I suppose you intended as private, but there is no such convention in Java. Use camelCase for fields and methods, regardless of visibility, that's the convention.

Another unusual thing on this line is the direct use of the public field combined instead of a getter. It's unusual because a getter would give more flexibility for future extensions, as methods can be overridden by derived classes, while fields cannot be.

In any case, the chained call and the public field are not your "fault". The GDX library designers decided to provide this usage, and I assume they knew what they were doing. If you meant your question as "should I break up the chain to local variables and call them like that?" like this:

Stage stage = _hud.getStage();
Camera camera = stage.getCamera();
Matrix4 projection = camera.combined;

No, that wouldn't change the fact of the coupling, this is still a chained call, just expanded, and this is worse than the compact writing style, with no benefits within this snippet.

If yes, what would be the pretty way to write the code?

Going just a tad further than @rolfl:

@Override
public void render(float delta) {
    Gdx.gl.glClearColor(1, 0, 0, 1);
    Gdx.gl.glClear(GL20.GL_COLOR_BUFFER_BIT);
    Stage stage = hud.getStage();
    batch.setProjectionMatrix(stage.getCamera().combined);
    stage.draw();
}

That is:

• Avoid the _ prefix for all fields
• Add a space after commas in parameter lists
• Extract shared code (hud.getStage()) to local variable

It's not much prettier than the original, but you asked for it.

What are the consequences of this?

The wiki page of Law of Demeter explains this in detail, but in a nutshell, chained method calls are tight coupling, unless the chained calls return the same object, such as in a fluent Builder or in a Java 8 Stream (thanks @sylvain-boisse, good point). It's ok that your class knows that hud has a Stage, but it's not ok that it knows that Stage has a Camera and further, that it has a projection named combined. The recommendation would be to provide a getCombinedCameraProjection method on hud, which would hide the class hierarchy, giving the designer the freedom to redesign the elements used behind that method (the Camera, etc), without affecting users of the API.

However, not following the Law of Demeter to the letter doesn't imply that this in itself is bad design. Applying LoD systemically everywhere would not be practical, as it could lead to an explosion of getters for the permutation of all uses of a Camera in this example. Multiple design recommendations often conflict (here encapsulation and usability), balancing them is extremely difficult. You have to choose between tradeoffs, and the designers of GDX have made their own, probably good choices.

Answer 2

Is it an ugly practice? No, not by itself. Method chaining like that is even expected in some circumstances - Java streams come to mind.

Your code is pretty clear, and the only changes I would suggest are:

• Using an _ underscore in Java is unconventional. It's a form of hungarian notation that should be removed

• I would pull the reuse of the first part of the method chain in to a variable declaration:

  _batch.setProjectionMatrix(_hud.getStage().getCamera().combined);
  _hud.getStage().draw();
  

  should be:

  Stage stage = _hud.getStage()
  _batch.setProjectionMatrix(stage.getCamera().combined);
  stage.draw();
  

  I would make that change because of the value reuse, not because of the chaining, though.