6
\$\begingroup\$

In my application, I need to allow the user to store variables. Variables can only be of specific types, but I effectively handle all variables the same way no matter the type. I would also like to avoid any casting. Therefore, I have several HashMaps for each variable type, which is causing lots of duplicated code. Is there a way to simplify my class?

public class VariableSet {

    private final Map<String, PlayerVariable> players;
    private final Map<String, NumberVariable> numbers;
    private final Set<String> notRemovable;
    private final Set<String> usedVariableNames;

    public VariableSet(){
        players = new HashMap<String, PlayerVariable>();
        numbers = new HashMap<String, NumberVariable>();
        notRemovable = new HashSet<String>();
        usedVariableNames = new HashSet<String>();
        addVariable("Current Player", new PlayerVariable(), players, false);
    }

    public PlayerVariable addPlayer(String variableName){
        PlayerVariable var = new PlayerVariable();
        addVariable(variableName, var, players, true);
        return var;
    }

    public NumberVariable addNumber(String variableName){
        NumberVariable var = new NumberVariable();
        addVariable(variableName, var, numbers, true);
        return var;
    }

    private <T extends Variable<?>> void addVariable(String variableName, T variable, Map<String, T> map, boolean removable){
        if (usedVariableNames.contains(variableName)){
            throw new VariableExistsException();
        }
        usedVariableNames.add(variableName);
        map.put(variableName, variable);
        if (!removable){
            notRemovable.add(variableName);
        }
    }

    public void removeNumber(String variableName){
        removeVariable(variableName, numbers);
    }

    public void removePlayer(String variableName){
        removeVariable(variableName, players);
    }

    private <T extends Variable<?>> void removeVariable(String variableName, Map<String, T> map){
        if (notRemovable.contains(variableName)){
            throw new VariableNotRemovableException();
        }
        usedVariableNames.remove(variableName);
        map.remove(variableName);
    }

    public boolean hasPlayerVariable(String variableName){
        return players.containsKey(variableName);
    }

    public boolean hasNumberVariable(String variableName){
        return numbers.containsKey(variableName);
    }

    public Set<String> getVariableNames(){
        return usedVariableNames;
    }

    public void setPlayer(String variableName, PlayerVariable player){
        players.put(variableName, player);
    }


    public void setNumber(String variableName, NumberVariable number){
        numbers.put(variableName, number);
    }
}

All variables extend the same abstract class, so using generics is probably the preferred method, but I can't figure out a way to create a data structure that automatically picks the correct variable type without casting.

\$\endgroup\$
5
\$\begingroup\$

The only way to do what you are trying to do, and remove the duplication, is really to cast.

What I would do is to use this kind of map:

private final Map<Class<? extends Variable>, Map<String, Variable>> variables;

To add something to the map, you could do:

variables.get(NumberVariable.class).put("key", value);

And to get:

return (NumberVariable) variables.get(NumberVariable.class).get("key");

You could even write a generic method:

private <T> T getVariable(Class<T> clazz, String key) {
    return clazz.cast(variables.get(clazz).get(key));
}

Note that this shouldn't even give any compiler warnings.


Leaking inner variable

Considering your getVariableNames method, imagine if I would call your code like this:

myVariableSet.getVariableNames.clear();

Whoops! I broke it!

The simple fix is this:

public Set<String> getVariableNames() {
    return new HashSet<>(usedVariableNames);
}

That is, return a copy of the data, then I can manipulate it all I want - I won't break anything!

I wouldn't use a usedVariableNames at all though and just do a map.containsKey(key) directly. If you want to make sure that all variable names are unique though, even if they are of different classes, then go ahead and keep this variable.

\$\endgroup\$
  • \$\begingroup\$ I think I'll do something similar to the generic cast you are describing. Thank you for the warning about my getVariableNames(). A use case for this class is to list out all variables, which means I need to keep usedVariableNames (I'm also interested in keeping them unique). \$\endgroup\$ – Nathan Merrill Feb 19 '15 at 21:33

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.