Game State Engine

Question

Can you improve my state engine which consists of the state engine based on an interface and an example menustate? It all works fine but seems a bit cumbersome in terms of the way I have to change states and also report on what state the engine is in. Can this be coded more elegantly?

public class StateEngine {
 //Pointer to cureent state
 private StateInterface mystate;
 StateEngine() {
 //Create new state of mainmenu at start
 setstate(new StateMainmenu());
 }
 //Set the state to the new state and capture a pointer
 final void setstate(StateInterface newstate) {
 this.mystate = newstate;
 }
 //Use the current state and point to its tellstate method
 final void tellstate() {
 mystate.tellstate();
 }
 //Use the current state, changestate method
 final void changestate(){
 mystate.changestate(this);
 }
 final void update(){
 mystate.update();
 }
 final void render(Graphics g){
 mystate.render(g);
 }
}

The interface

public interface StateInterface {
 void changestate(StateEngine currentstate);
 void tellstate();
 void update();
 void render(Graphics g);
}

Example state for the menu

public class StateMainmenu implements StateInterface {
 //private StateEngine state;
 static String[] mainmenu = new String [] {"Start","Options","Quit"};
 static int menuselection;
 public BufferedImage menubackgroundscreen;
 private int myfontsize=60;
 private int width=Inferno.appwidth, height=Inferno.appheight;
 StateMainmenu(){
 System.out.print("Initialising Menu, "); 
 menuselection=0;
 menubackgroundscreen=new BufferedImage(width,height,BufferedImage.TYPE_INT_ARGB);
 Graphics g=menubackgroundscreen.getGraphics();
 }
 @Override
 public void changestate(StateEngine currentstate) {
 System.out.println("Changing from menu");
 switch(menuselection){
 case 0: System.out.println("Option 1 Selected");
 currentstate.setstate(new Selectionstate(currentstate)); 
 break;
 case 1: System.out.println("Option 2 Selected");
 break;
 case 2: System.out.println("Option 3 Selected");
 Inferno.running=false;
 break;
 }
 }
 @Override
 public void tellstate() {
 System.out.println("Menu state active");
 }
 public void nextitem(){
 menuselection++;
 if (menuselection==mainmenu.length) menuselection=0;
 }
 public void previtem(){
 menuselection--;
 if (menuselection==-1) menuselection=mainmenu.length-1;
 }
 @Override
 public void update() {
 if (InputManager.keydown){
 nextitem();
 InputManager.keydown=false;
 }
 if (InputManager.keyup){
 previtem();
 InputManager.keyup=false;
 }
 if (InputManager.keyenter){
 InputManager.keyenter=false;
 Inferno.gamestate.changestate();
 }
 }
 @Override
 public void render(Graphics g) {
 g.drawImage(menubackgroundscreen,0,0, width, height, null);
 for (int i=0; i<mainmenu.length;i++){
 g.setFont(new Font("Arial",Font.BOLD,myfontsize)); 
 g.setColor(Color.DARK_GRAY);
 if (menuselection==i) g.setColor(Color.WHITE);
 g.drawString(mainmenu[i], 100, 250+(i*myfontsize)); 
 } 
 }
}

Answer 1

There are more than a few things that are concerning in your code. For a start, you have a combination of static, and instance logic happening in your StateMainmenu:

static String[] mainmenu = new String [] {"Start","Options","Quit"};
static int menuselection;
public BufferedImage menubackgroundscreen;
private int myfontsize=60;
private int width=Inferno.appwidth, height=Inferno.appheight;

You have non-final non-private statics, and I presume they are accessed from all over the place. Additionally, the menubackgroundscreen is public?

Your classes should be properly encapsulated, and state changes should be only possible from triggers that are managed from inside the class.

The System.out.println() messages in places like:

@Override
public void tellstate() {
 System.out.println("Menu state active");
}

are misleading. Why are you debugging in a method like this? Is it debugging, or is that a core output of the program? Is the only thing needed to do a println? The method should be called tellState as well, with a capital S.

The methods:

public void nextitem(){
 menuselection++;
 if (menuselection==mainmenu.length) menuselection=0;
}
public void previtem(){
 menuselection--;
 if (menuselection==-1) menuselection=mainmenu.length-1;
}

manipulate a static field, but they are instance methods. Also, the use of a modulo would help (and a capital I for Item):

The nextItem (with a capital I) would be:

public void nextItem(){
 menuselection = (menuselection + 1) % mainmenu.length;
}

prevItem (with a capital I) is more complicated, but also:

public void prevItem(){
 menuselection = (menuselect + mainmenu.length - 1) % mainmenu.length;
}

The update() method seems contrived, and will also do multiple things depending on multiple states. Should the methods just return instead of falling through to the next check. Odd, but the current code could be right, just unusual. The way they call nextItem and prevItem implies those methods can be private.

Now, about the overall state system.

It's hard to get a feel for the actual state manipulation. You only have one state, and it is controlled mostly by static manipulations. The actual state changes are 'wrapped' in a class you do not show to us:

currentstate.setstate(new Selectionstate(currentstate));

What's this 'Selectionstateclass? (and why is it not calledSelectionState`?).