5
\$\begingroup\$

Most of my experience is with Objective-C, so I am relatively new to Java inheritance. I understand that there are concepts such as abstract classes and interfaces, but I am still not totally sure when to use them. I would really like feedback on the structure of my inheritance in the following code. I am most concerned about the readability of the code, and following Java best practices.

Here I have the code for the menus of my game. Typically, when a user presses a key or clicks a button, the menu will appear on screen. I have a number of different types of menus that have different buttons and display different information. I should note that I am using libGDX Tables for the layout of the menus.

First, here is the base class that all the other menu classes will subclass:

BZMenuTable

public class BZMenuTable {

    protected Table menuTable;
    protected final Skin skin;
    private final Stage hudStage;
    private boolean isOpen;

    public BZMenuTable(Skin skin, Stage hudStage) {
        this.skin = skin;
        this.hudStage = hudStage;
        this.isOpen = false;
    }

    public boolean isOpen() {
        return this.isOpen;
    }

    public void open() {
        this.isOpen = true;
        this.menuTable = new Table(this.skin);
        this.hudStage.addActor(this.menuTable);
    }

    public void build() {
        //overridden by subclasses
    }

    public void close() {
        this.isOpen = false;
        this.menuTable.remove();
    }

}

Next I will provide two different examples of menu subclasses. This first one has pages:

HelpMenu

public class HelpMenu extends BZMenuTable {

    private int pageNumber;
    private HashMap<Integer, ArrayList<String>> pageContent = new HashMap<Integer, ArrayList<String>>();
    private final NinePatch menuBackground;
    private final float stageWidth;
    private final float stageHeight;

    public HelpMenu(Skin skin, Stage hudStage, NinePatch menuBackground, float stageWidth, float stageHeight) {
        super(skin, hudStage);
        this.pageContent = this.getHelpPages();
        this.pageNumber = 0;
        this.menuBackground = menuBackground;
        this.stageWidth = stageWidth;
        this.stageHeight = stageHeight;
    }

    @Override
    public void build() {
        this.build(this.pageNumber);
    }

    public void build(int pageNumber) {

        super.menuTable.clear();

        TextButton previousButton = new TextButton("Prev", super.skin);
        previousButton.addListener(new ClickListener() {
            @Override
            public void clicked(InputEvent event, float x, float y) {
                HelpMenu.this.pageNumber = Math.max(HelpMenu.this.pageNumber - 1, 0);
                HelpMenu.this.build(HelpMenu.this.pageNumber);
            }
        });

        TextButton nextButton = new TextButton("Next", skin);
        nextButton.addListener(new ClickListener() {
            @Override
            public void clicked(InputEvent event, float x, float y) {
                HelpMenu.this.pageNumber = Math.min(HelpMenu.this.pageNumber + 1, HelpMenu.this.pageContent.size() - 1);
                HelpMenu.this.build(HelpMenu.this.pageNumber);
            }
        });

        this.menuTable.setBackground(new NinePatchDrawable(this.menuBackground));
        this.menuTable.setPosition(this.stageWidth/10, this.stageHeight/10);
        this.menuTable.setWidth((float)(this.stageWidth * 0.8));
        this.menuTable.setHeight((float)(this.stageHeight * 0.8));

        for (String string : this.pageContent.get(this.pageNumber)) {
            //Another way to do paragraphs here
            Label label = new Label(string, skin);
            label.setWrap(true);
            //it just sets it to the width of the buttons below anyway
            //and if those buttons arent present, it sets the max width to some really small number
            //this.menuTable.add(label).maxWidth((int)(this.stageWidth * 0.9)).fill();
            this.menuTable.add(label).fill();
            this.menuTable.row();
            this.menuTable.add("-");
            this.menuTable.row();
        }

        Table pageButtons = new Table(super.skin);
        pageButtons.add(previousButton).bottom().width(this.stageWidth/4).height(this.stageHeight/10).fill();
        pageButtons.add(nextButton).bottom().width(this.stageWidth/4).height(this.stageHeight/10).fill();
        this.menuTable.add(pageButtons);  
    }

    private HashMap<Integer, ArrayList<String>> getHelpPages() {
        HashMap<Integer, ArrayList<String>> helpPages = new HashMap<Integer, ArrayList<String>>();

        int pageNumber = 0;
        ArrayList<String> firstPageArray = new ArrayList<String>();
        firstPageArray.add("How to Play");
        firstPageArray.add("Press the Menu button to open the menu for the tile you are standing on. The resources in your inventory are shown in the top left corner.");
        firstPageArray.add("Place buildings to gain resources over time.  When you open the Menu and select a building, you can see the resources required to build it.");
        firstPageArray.add("Each building produces a different resource, and some gain you influence instead. If your inventory is full, you can store resources in matching buildings.");
        helpPages.put(pageNumber, firstPageArray);
        pageNumber++;

        ArrayList<String> secondPageArray = new ArrayList<String>();
        secondPageArray.add("Trading");
        secondPageArray.add("You start with a small amount of resources and gold, so it may not be possible to build buildings right away.");
        secondPageArray.add("Travel around to different Regions and you will find Capital Cities represented by red towers.");
        secondPageArray.add("Each of them will buy and sell certain resources. Trade with them in order to get the resources you need to start building.");
        secondPageArray.add("The player that trades most with a Capital will have influence over it.");
        helpPages.put(pageNumber, secondPageArray);
        pageNumber++;

        ArrayList<String> influencePageArray = new ArrayList<String>();
        influencePageArray.add("Influence");
        influencePageArray.add("When you build a building on a tile, you gain influence over it. Buildings can never be removed.");
        influencePageArray.add("Some buildings gain influence over surrounding tiles. Other players cannot build on tiles that you control.");
        influencePageArray.add("When you control more than half of the tiles in a region, you become the owner of that region.");
        influencePageArray.add("Gain control of as many regions as possible to win.");
        helpPages.put(pageNumber, influencePageArray);
        pageNumber++;

        ArrayList<String> netherPageArray = new ArrayList<String>();
        netherPageArray.add("The Nether Zone");
        netherPageArray.add("Outside the edges of the World, you will find the Nether Zone.");
        netherPageArray.add("These fragmented regions may only be visited once, for each time you visit there, you will see a new region.");
        netherPageArray.add("It costs energy to move in the Nether Zone, and even more energy to move between Nether Zone regions. If you run out you will be transported back to the regular world.");
        netherPageArray.add("Prices are greatly increased in the Nether Zone due to limited supply.");
        helpPages.put(pageNumber, netherPageArray);

        return helpPages;
    }

}

This last one needs to be updated when the game is updated in order to keep its information accurate:

ResourceMenu

public class ResourceMenu extends BZMenuTable {

    private final MainGame game;
    private final float stageWidth;
    private final float stageHeight;
    private Label resourceLabel;

    public ResourceMenu(Skin skin, Stage hudStage, MainGame game, float stageWidth, float stageHeight) {
        super(skin, hudStage);
        this.game = game;
        this.stageWidth = stageWidth;
        this.stageHeight = stageHeight;
    }

    @Override
    public void build() {
        TextButton giveResourcesButton = new TextButton("Give", skin);
        giveResourcesButton.addListener(new ClickListener() {
            @Override
            public void clicked(InputEvent event, float x, float y) {
                ResourceMenu.this.game.playerGaveAllResources();
                ResourceMenu.this.close();
            }
        });

        TextButton takeResourcesButton = new TextButton("Take", skin);
        takeResourcesButton.addListener(new ClickListener() {
            @Override
            public void clicked(InputEvent event, float x, float y) {
                ResourceMenu.this.game.playerTookAllResources();
                ResourceMenu.this.close();
            }
        });

        this.menuTable.setFillParent(true);
        this.menuTable.add(giveResourcesButton).width(this.stageWidth/4).height(this.stageHeight/10).fill();
        this.menuTable.row();
        this.menuTable.add(takeResourcesButton).width(this.stageWidth/4).height(this.stageHeight/10).fill();
        this.menuTable.row();
        this.menuTable.add(this.game.getPlayerPositionTile().getResourceProduced().name() + ":");
        this.menuTable.row();
        this.resourceLabel = new Label(String.valueOf(this.game.getPlayerPositionTile().getNumberResourcesForPickup()), this.skin);
        this.menuTable.add(resourceLabel);
    }

    public void update() {
        this.resourceLabel.setText(String.valueOf(this.game.getPlayerPositionTile().getNumberResourcesForPickup()));
    }

}

Here is the method where the HelpMenu is created by the GameScreen:

private void buildHelpMenu() {
    if (!this.isAnyMenuOpen()) {
        this.helpMenu = new HelpMenu(this.skin, this.libGDXGame.hudStage, this.libGDXGame.menuBackground, LibGDXGame.STAGE_WIDTH, LibGDXGame.STAGE_HEIGHT);
        this.helpMenu.open();
        this.helpMenu.build();
    } else {
        this.helpMenu.close();
    }
}

And here is the method where the ResourceMenu is created by the GameScreen:

private void buildResourceMenu() {
    if (this.resourceMenu == null || !this.resourceMenu.isOpen()) {
        this.resourceMenu = new ResourceMenu(this.skin, this.libGDXGame.hudStage, this.game, LibGDXGame.STAGE_WIDTH, LibGDXGame.STAGE_HEIGHT);
        this.resourceMenu.open();
        this.resourceMenu.build();
    } else {
        this.resourceMenu.close();
    }
}

Should I have an interface here for the methods isOpen, open, close, and build? Or maybe it should be an abstract class that forces the subclasses to provide an implementation for the build method? Or both? The problem I see with this approach is that it seems to add unnecessary files to the code. However, like I said above, I am new to Java and I want to be following the best practices even if they don't completely make sense to me.

\$\endgroup\$
2
\$\begingroup\$

Checking if something is on stage

In your parent class you have:

public boolean isOpen() {
    return this.isOpen;
}

This can be replaced by menuTable.hasParent()

setPage method?

HelpMenu.this.pageNumber = ...;
HelpMenu.this.build(HelpMenu.this.pageNumber);

This is some duplciated code. I'd recommend creating a void setPage(int page) method to do this.

Percent size

I'm not a big fan of this code:

this.menuTable.setPosition(this.stageWidth/10, this.stageHeight/10);
this.menuTable.setWidth((float)(this.stageWidth * 0.8));
this.menuTable.setHeight((float)(this.stageHeight * 0.8));

I'd recommend using an outer table, and add an inner table to that. For the inner table you can then use Value.percent to set padding and size. See the LibGDX Table documentation for an example of how to use percent values with Value.

Btw, you can avoid the (float) cast:

this.menuTable.setWidth(this.stageWidth * 0.8f);

Default values

Table pageButtons = new Table(super.skin);
pageButtons.add(previousButton).bottom().width(this.stageWidth/4).height(this.stageHeight/10).fill();
pageButtons.add(nextButton).bottom().width(this.stageWidth/4).height(this.stageHeight/10).fill();

This contains some code duplication. You can use Table defaults to avoid them.

Something like this:

Table pageButtons = new Table(super.skin);
pageButtons.defaults().bottom().width(this.stageWidth/4).height(this.stageHeight/10).fill();
pageButtons.add(previousButton);
pageButtons.add(nextButton);

Additionally, I'd again recommend using percent values to avoid the division with stage size.

Help pages

Your getHelpPages returns a HashMap<Integer, ArrayList<String>>

First of all, use the interface to return Map<Integer, List<String>>.

However, as the keys for this map are essentially indexes, you can use List<List<String>>.

In fact, as you don't need any dynamic resizability, you can use String[][], which will allow much easier construction of the help pages.

private String[][] getHelpPages() {
    return new String[][]{
            {
                    "This is",
                    "the first",
                    "help page"
            },
            {
                    "This is another",
                    "or more specifically",
                    "the second",
                    "help page"
            },
            {
                    "This is one more",
                    "also known as",
                    "the third",
                    "help page"
            },
    };
}    

Overall design / Organization of the menus

In your usage for how you actually use these classes, you are always calling menu.build() after menu.open(). Because your open() method does this:

this.menuTable = new Table(this.skin);
this.hudStage.addActor(this.menuTable);

You're creating the menu table in the open method, and you also add it to the stage.

Instead, I would recommend that you create the menuTable only once, in the constructor.

The parent constructor can do

this.menuTable = new Table(this.skin);

And the child constructors can then fill it with content (what you are currently doing in the build() method). The build() method essentially does the job of a constructor right now.

With this change, you can create all menus once (in the constructor for your LibGDX Screen perhaps? Or when you first need the menus).

You can then use a toggle method in your parent class:

public void toggle() {
    if (this.isOpen()) {
        // add to stage
    } else {
        // remove from stage
    }
}

Which allows your usage of these menus to be:

private void buildHelpMenu() {
    this.helpMenu.toggle();
}
\$\endgroup\$
4
\$\begingroup\$

I'll focus my review on this small piece of code :

public void build() {
    //overridden by subclasses
}

This tell me that your class BZMenuTable should be an abstract class. If this is only implemented in the subclass, that means you never really need BZMenuTable, other than to provide method that need to be there for all the inherited classes.

Since you do implement some method in the parent class, that means it's not an interface (you could still have an interface to specified the methods commons to all), but should be an abstract class. What that means is, you cannot instatiate an instance of this class, only one of his children.

public abstract class BZMenuTable {

    protected Table menuTable;
    protected final Skin skin;
    private final Stage hudStage;
    private boolean isOpen;

    public BZMenuTable(Skin skin, Stage hudStage) {
        this.skin = skin;
        this.hudStage = hudStage;
        this.isOpen = false;
    }

    public boolean isOpen() {
        return this.isOpen;
    }

    public void open() {
        this.isOpen = true;
        this.menuTable = new Table(this.skin);
        this.hudStage.addActor(this.menuTable);
    }

    public abstract void build();

    public void close() {
        this.isOpen = false;
        this.menuTable.remove();
    }

}

It's now clear that the build() method should be implemented in all children.

\$\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.