current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

Before I used reflection to invoke a state based on the string. It looked like this.

public void setState (String state) {
    try {
        this.state = (State) Class.forName("state"+state).getConstructor().newInstance();
    } catch (Exception e) {
        System.out.println ("Tried to set state to " + state.toString() + ", but no such state exists.");
    }        
}

Addition to not invalidate answer. It is supposed to say ...forName("state."+state)... Notice the period in "state.".

However, it feels wrong to use reflection when I know all the State the project can be in. They are:

  • Title
  • NewGame
  • LoadGame
  • Credits
  • HighScore
  • World

Rendering the most straight-forward solution to be along these lines.

private  void setState (String state) {
    switch (state) {
        case "Title":
            this.state = new Title ();
            break;
        case "NewGame":
            this.state = new NewGame ();
            break;
        case "LoadGame":
            this.state = new LoadGame ();
            break;
        case "Credits":
            this.state = new Credits ();
            break;
        case "HighScore":
            this.state = new HighScore ();
            break;
        case "World":
            this.state = new World ();
            break;
        default:
            System.out.println ("Tried to set state to " + state.toString() + ", but no such state exists.");
    }
}

Is it possible to clean that up?

For example could instance-mapping be a solution?

There is a very similiar situation with the handle method. In the reflection case it looks like this.

public void handle (Event event) {
    try {
        this.getClass().getMethod(event.getMethod(), String.class).invoke(this, event.getArgument());
    } catch (Exception e) {
        state.handle (event);
    }
}

If the method can't be invoked in this class then it will be invoked in the state instead. Meaning the exception can be caught for that reason.

However, it does not look as neat if I were to use mapping or a switch-case clause here. 

public void handle (Event event) {
    switch (event.getMethod()) {
        case "setState":
            setState (event.getArgument());
            break;
        case "loadGame":
            loadGame (event.getArgument());
            break;
        case "logDebug":
            logDebug (event.getArgument());
            break;
        case "exitGame":
            exitGame (event.getArgument());
            break;
        default:
            state.handle(event);
            break;
    }
    logDebug ("Event handled: " + event.toString());
}

Can something similiar be used in these two cases or is it fine to use reflection?

share|improve this question
    
The desire to improve code is implied for all questions on this site. Question titles should reflect the purpose of the code, not how you wish to have it reworked. See How to Ask. –  Jamal♦ Jun 29 at 23:38

2 Answers 2

up vote 5 down vote accepted 
    this.state = (State) Class.forName("state"+state).getConstructor().newInstance();

This looks for a class named "stateWhatever", which isn't what you're doing below. Let's assume it's an error which happened when you copied the code.

private  void setState (String state) {
    switch (state) {
        case "Title":
            this.state = new Title ();
            break;
        case ....................
        default:
            System.out.println ("Tried to set state to " + state.toString() + ", but no such state exists.");
    }
}

Yes, this is going to get pretty long. But don't you simply separate the setting of the state from its computation?

 private State getState() {
    switch (state) {
        case "Title": 
            return new Title ();
        case ....

Here, you save one line, you could save another one if your conventions allow it and that's the optimum.

In case you can precreate your states, then simply

private static final Map<String, State> stateMap;
static {
     Map<String, State> map = new Map<>();
     map.put("Title", new Title()):
     ...
     stateMap = Collections.unmodifieableMap(map);
}

would do. The usage is obvious.

In case you can't precreate your states, then you could create a

private static final Map<String, Class<? extends State>> stateClassMap;

and use reflection for the instance creation.

You're having many implementation of state and each of them has a no-args constructor. This looks like an enum.

    System.out.println ("Tried to set state to " + state.toString() + ", but no such state exists.");

You know that's no proper error handling, right?

share|improve this answer
    
"state." is what it is supposed to be yes. I will check into precreation of the states. That error handling is just a quick write. I saw no reason to paste all of the error checking here now. –  Emz Jun 30 at 0:08
    
@Emz OK, then simply write "`handleError(state);" instead of the printing. –  maaartinus Jun 30 at 0:25
    
I added another example as well, similiar, but not the same. If you want to draw something from it. –  Emz Jun 30 at 0:44
up vote 4 down vote

I would solve this problem this like:

  1. Create an array of the names of classes that you might set the state to.

  2. Iterate through this array

  3. If the state string passed in via argument is equal to the string that we are currently on in the iteration, go to 5

  4. Go to 2

  5. Set this.state to an instance of the class referenced by the string.

Here is my solution:

String[] stateClasses = {"Title", "NewGame", "LoadGame", "Credits", "HighScore", "World"};
boolean foundStateClass = false;

for(String stateClass : stateClasses) {
    if(stateCass.equals(state)) {
        this.state = Class.forName(stateClass);
        foundStateClass = true;
    }
}

if(!foundStateClass) {
    System.out.println ("Tried to set state to " + state.toString() + ", but no such state exists.");
}

Sorry if this is repeat of what you did in your first solution.

share|improve this answer
    
Wouldn't a Set<E> do the trick better than a list? Can use contains (Object o), meaning I don't need to have the foundStateClass in there? –  Emz Jun 30 at 0:18
    
@Emz Sure! I hadn't even thought of a Set at the time. However, it wouldn't be as easy to setup the Set with the strings, unless you kept the array I wrote and then setup another loop to put all the strings from the array into the Set. Also, I don't know about the efficiency of contains compared to setting and checking a boolean value. –  SirPython Jun 30 at 0:22
    
In your first line you mention "instance-mapping". So why don't you use a Map? Maybe to prevent early class loading? –  maaartinus Jun 30 at 0:23
    
@maaartinus I mentioned "instance-mapping" because the OP mentioned it in their post. When I thought of mapping, I thought of creating an array of strings and associating classes to each string. I guess that wasn't the best choice of words on my part. –  SirPython Jun 30 at 0:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.