World Map Viewer with LibGDX

I am working on a game where the player can walk around a world comprised out of a grid of tiles. The World object contains a HashMap of Region objects which each has a HashMap of Tile objects.

For the libGDX side of things, the Game object creates the different Screens of the game, and switching between Screens is handled by calls to the Stage of the Game object.

The following code is the code for the world map for the game. When the user presses the Map button, its swipe recognizer is disabled and it tells the Game to tell its Stage to load the MapScreen.

There is a lot of coupling going on with each Screen and the Game. I know that this is a problem, but I don't know the best way to go about fixing it. I am happy to hear any and all criticism about the code.

```
public class MapScreen extends ScreenAdapter {

//used to center the content of the screen
private final int screenX = 15;
private final int screenY = 120;

private final int tileSize = 15;
private final int tileSpacing = 3;
private final ShapeRenderer shape = new ShapeRenderer();

private final Skin skin;
private Table table;

private final SCGame game;
private final World gameWorld;

public MapScreen(SCGame game, World gameWorld) {
this.skin = new Skin(Gdx.files.internal("data/uiskin.json"));
this.game = game;
this.gameWorld = gameWorld;
this.enableCameraPanning();
this.createButtons();
}

private void enableCameraPanning() {
this.game.cameraPanner.setEnabled(true);
}

private void createButtons() {
TextButton exitButton = new TextButton("Exit", skin);
exitButton.addListener(new ClickListener() {
@Override
public void clicked(InputEvent event, float x, float y) {
MapScreen.this.clickedMapButton();
}
});

this.table = new Table();
table.setPosition((float)(-SCGame.STAG

It seems like these variables might be better off as constants:

private final int screenX = 15;
    private final int screenY = 120;
    private final int tileSize = 15;
    private final int tileSpacing = 3;

Adding static to them and renaming them to use CONSTANT_CASE will take care of that.

private final ShapeRenderer shape = new ShapeRenderer();

Possible Memory Leak! You are never calling shape.dispose(); I would recommend creating this ShapeRenderer in your SCGame class and pass it to the screen(s). Then your SCGame can take care of properly disposing it as well.

Alternatively, you need to take care of disposing your screens yourself and put the disposing on it in a @Override public void dispose() method.

I would make your Table final and create it inside the constructor instead of putting it inside a createButtons method. You're only calling createButtons from the constructor currently.

You are right about the coupling with SCGame. That's not ideal.

What I am trying to do in LibGDX projects is to put the things required by other screens into some kind of a home-made GameContext class. Sort of like this:

public class GameContext {

    private final Camera camera;
    private final Skin skin;
    private final Stage stage;

    public GameContext(Camera camera, Skin skin, Stage stage) {
        this.camera = camera;
        this.skin = skin;
        this.stage = stage;
    }

    public Stage getStage() {
        return stage;
    }

    public Camera getCamera() {
        return camera;
    }

    public Skin getSkin() {
        return skin;
    }

}

Passing an instance of GameContext around to your screens will keep them independent on your SCGame class. Of course you can also add other resources to this class that you want your screens to have.

Another approach is also to extract an interface that contains the methods you want to have access to, such as:

public interface GameInterface {
    void backToMainMenu();
}

Then you could have your SCGame implement this interface, and pass around the interface to your classes.

public MapScreen(GameInterface game, World gameWorld) {
    this.game = game;
    ...
}

table.setPosition((float)(-SCGame.STAGE_WIDTH/2.3), (float)(-SCGame.STAGE_HEIGHT/2.2));

This is some strange code that you really shouldn't need to have. Your table should position itself correctly to stretch across the screen without this code.

After a bit of testing, I found that the current code centers the button, not puts it into the bottom-right corner. The correct way is either:

this.table = new Table();
this.table.setFillParent(true);
this.table.add(mapButton).expand().right().bottom();
this.game.stage.addActor(this.table);

Or:

this.table = new Table();
this.table.setFillParent(true);
this.table.right().bottom();
this.table.add(mapButton);
this.game.stage.addActor(this.table);

Then you probably will want some margin between the button and the edge of the screen as well, which you can do by adding .pad(10) at the end of the table.add(mapButton) line.

@Override
public void render (float delta) {  
    draw();
}

Well, that was a useful method!

Please do us a favor and move all your draw() code to this render() method instead. There's really no need for both.

private Color colorForType(TileType type) {
    switch(type) {
        case GRASS:
            return Color.GREEN;

So you are associating each of your TileType members with a color?

Use enum methods, my friend!

public enum TileType {
    GRASS(Color.GREEN),
    FOREST(Color.OLIVE),
    ...;

    private final Color color;

    private TileType(Color color) {
        this.color = color;
    }

    public Color getColor() {
        return this.color;
    }
}

Then instead of colorForType(tileType) you can call tileType.getColor() (unless tileType is null of course, but I hope you are avoiding that.

StackExchange Code Review Q#82173, answer score: 8