Singleton has logic and state, and logic has state

Question

------edit---------

note: this is for a MUD client, so the commands are issued to the MUD game server

-----end edit--------

For an overview, here's the structure of the project:

NetBeansProjects/TelnetConsole/src/
├── connection.properties
└── telnet
    ├── connection
    │   ├── CharacterDataQueueWorker.java
    │   ├── ConsoleReader.java
    │   ├── InputStreamWorker.java
    │   └── PropertiesReader.java
    ├── Controller.java
    └── game
        ├── Command.java
        ├── PlayerCharacter.java
        ├── PlayerFlags.java
        ├── PlayerLogic.java
        ├── PlayerStats.java
        └── RegexWorker.java

3 directories, 12 files

I'm fairly sure that PlayerFlags and PlayerStats, if not PlayerCharacter itself, deserve Builders. This, very basic, MUD client has PlayerCharacter as Singleton to ensure that the correct (the only) player character is referenced.

I've resorted to making the other classes beans because I seem to have a sort of infinitite regression where PlayerCharacter has PlayerFlags (booleans) and PlayerStats (integers), as well as PlayerLogic and the RegexWorker. I would much rather use builders, or factories, to clarify and make explicit the state of the player character and the commands.

Ideally, PlayerLogic and RegexWorker should, I think, strictly have static methods, but for now, these classes also are distorted beans, which is ok.

The infinite, or too deep for me, at least, regression is that PlayerLogic and RegexWorker each have references to PlayerStats and PlayerFlags. RegexWorker sets flags on the PlayerCharacter Singleton, while PlayerLogic both sets and reads the flags. (Yes, the logic class shouldn't change the state of the `PlayerCharacter', but that's ok for now.) It seems to be circular composition, or at least flawed in some way.

It becomes a problem of initialization. While PlayerStats and PlayerFlags have default settings, its never quite explicit, to me, which reference is being used at any given time. As a kludge, the utility type classes, RegexWorker and PlayerLogic, each get a fresh reference to the current state through the PlayerCharacter Singleton for each opportunity to do so, and similarly refresh the state of PlayerCharacter whenever possible.

Without this kludge, the program refuses to run, generating, if I recall correctly, a null-pointer because PlayerStats and PlayerFlags each have null references, not having not initialized in the Singleton. (The regex worker actually initializes the state of the player character with data from the MUD server.)

Because of the initialization paradox, these classes should have private constructors and at least a Builder each, if not a static factory method to create new instances, if only to make it easier for me to understand. I would rather that state be immutable, but then it's just awkward, and very verbose. However, each time I try to make these classes small and immutable it just gets more complex.

The controller invokes PlayerCharacter.processRemoteOutput as necessary (when it receives a message from the CharacterDataQueueWorker).

code:

package telnet.game;

import java.util.Queue;
import java.util.logging.Logger;

public enum PlayerCharacter {

    INSTANCE;  //only one player can play the client
    private final static Logger log = Logger.getLogger(PlayerCharacter.class.getName());
    private PlayerStats stats = new PlayerStats();
    private PlayerFlags flags = new PlayerFlags();

    private PlayerCharacter() {
    }

    public Queue<Command> processRemoteOutput(String remoteOutputMessage) {
        log.fine("trying to process...");
        RegexWorker regexWorker = new RegexWorker();
        regexWorker.parseAndUpdatePlayerCharacter(remoteOutputMessage);
        PlayerLogic playerLogic = new PlayerLogic();
        Queue<Command> newCommands = playerLogic.doLogic();
        for (Command c : newCommands) {
            log.fine(c.toString());
        }
        return newCommands;
    }

    public PlayerStats getStats() {
        return stats;
    }

    public void setStats(PlayerStats stats) {
        this.stats = stats;
    }

    public PlayerFlags getFlags() {
        log.fine(flags.toString());
        return flags;
    }

    public void setFlags(PlayerFlags flags) {
        this.flags = flags;
    }
}package telnet.game;

import java.util.List;
import java.util.Map.Entry;
import java.util.Observable;
import java.util.logging.Logger;

public class PlayerFlags {

    private final static Logger log = Logger.getLogger(PlayerFlags.class.getName());
    private boolean backstab = false;
    private boolean heartplunge = false;
    private boolean enervate = false;
    private boolean confuse = false;
    private boolean corpse = false;
    private boolean loggedIn = true;
    private boolean doping = false;
    private boolean healing = false;

    public PlayerFlags() {
    }

    PlayerFlags(List<Entry> flagsEntries) {
        String key = null;
        boolean val = false;


        switch (key) {
            case "loggedIn":
                loggedIn = val;
                break;
            case "confuse":
                confuse = val;
                break;
        }
    }

    public boolean isBackstab() {
        return backstab;
    }

    public void setBackstab(boolean backstab) {
        this.backstab = backstab;
    }

    public boolean isHeartplunge() {
        return heartplunge;
    }

    public void setHeartplunge(boolean heartplunge) {
        this.heartplunge = heartplunge;
    }

    public boolean isEnervate() {
        return enervate;
    }

    public void setEnervate(boolean enervate) {
        this.enervate = enervate;
    }

    public boolean isConfuse() {
        return confuse;
    }

    public void setConfuse(boolean confuse) {
        this.confuse = confuse;
    }

    public boolean isCorpse() {
        return corpse;
    }

    public void setCorpse(boolean corpse) {
        this.corpse = corpse;
        log.fine("corpse\t" + this.corpse);
    }

    public boolean isLoggedIn() {
        return loggedIn;
    }

    public void setLoggedIn(boolean loggedIn) {
        this.loggedIn = loggedIn;
    }

    public boolean isDoping() {
        return doping;
    }

    public void setDoping(boolean doping) {
        this.doping = doping;
    }

    public boolean isHealing() {
        return healing;
    }

    public void setHealing(boolean healing) {
        this.healing = healing;
    }

    public String toString() {
        return "corpse\t" + corpse;
    }
}
package telnet.game;

import java.util.List;
import java.util.Map.Entry;

public class PlayerStats {

    private int hp = 0;
    private int cp = 0;
    private int adrenaline = 0;
    private int endorphine = 0;
    private int berserk = 0;
    private int none = 0;
    private int darts = 0;
    private int blood = 0;
    private int grafts = 0;

    public PlayerStats() {
    }

    public PlayerStats(List<Entry> stringEntries) {

        String key = null, val = null;
        for (Entry e : stringEntries) {
            key = e.getKey().toString();
            key = key.toLowerCase();
            val = e.getValue().toString();
            if (key.contains("hp")) {
                key = "hp";
            }
            if (key.contains("cp")) {
                key = "cp";
            }
            if (key.contains("adrenaline")) {
                key = "adrenaline";
            }
            if (key.contains("endorphine")) {
                key = "darts";
            }
            if (key.contains("berserk")) {
                key = "darts";
            }
            if (key.contains("none")) {
                key = "none";
            }
            if (key.contains("blood")) {
                key = "blood";
            }
            if (key.contains("grafts")) {
                key = "grafts";
            }
            switch (key) {
                case "hp":
                    hp = Integer.parseInt(val);
                    break;
                case "cp":
                    cp = Integer.parseInt(val);
                    break;
                case "adrenaline":
                    adrenaline = Integer.parseInt(val);
                    break;
                case "endorphine":
                    endorphine = Integer.parseInt(val);
                    break;
                case "berserk":
                    berserk = Integer.parseInt(val);
                    break;
                case "none":
                    none = Integer.parseInt(val);
                    break;
                case "darts":
                    darts = Integer.parseInt(val);
                    break;
                case "blood":
                    blood = Integer.parseInt(val);
                    break;
                case "grafts":
                  grafts=Integer.parseInt(val);
                    break;
            }
        }
    }


    public int getHp() {
        return hp;
    }

    public void setHp(int hp) {
        this.hp = hp;
    }

    public int getCp() {
        return cp;
    }

    public void setCp(int cp) {
        this.cp = cp;
    }

    public int getAdrenaline() {
        return adrenaline;
    }

    public void setAdrenaline(int adrenaline) {
        this.adrenaline = adrenaline;
    }

    public int getEndorphine() {
        return endorphine;
    }

    public void setEndorphine(int endorphine) {
        this.endorphine = endorphine;
    }

    public int getBerserk() {
        return berserk;
    }

    public void setBerserk(int berserk) {
        this.berserk = berserk;
    }

    public int getNone() {
        return none;
    }

    public void setNone(int none) {
        this.none = none;
    }

    public int getDarts() {
        return darts;
    }

    public void setDarts(int darts) {
        this.darts = darts;
    }

    public int getBlood() {
        return blood;
    }

    public void setBlood(int blood) {
        this.blood = blood;
    }

    public int getGrafts() {
        return grafts;
    }

    public void setGrafts(int grafts) {
        this.grafts = grafts;
    }

    public String toString() {
        return "\n\nhp\t" + hp + "\tcp\t" + cp + "\tadrenaline\t" + adrenaline
                + "\nendorphine\t" + endorphine + "\t\tberserk\t" + berserk
                + "\nenemy\t" + none;
    }
}
package telnet.game;

import java.util.LinkedList;
import java.util.Queue;
import java.util.logging.Logger;

public class PlayerLogic {

    private final static Logger log = Logger.getLogger(PlayerLogic.class.getName());
    private PlayerCharacter playerCharacter = PlayerCharacter.INSTANCE;
    private PlayerFlags flags = new PlayerFlags();

    public PlayerLogic() {
    }

    private Queue<Command> confuse() {
        Queue<Command> commands = new LinkedList<>();
        Command confuse = new Command("confuse");
        Command backstab = new Command("backstab");
        Command heartplunge = new Command("heartplung");
        Command enervate = new Command("enervate");
        commands.add(confuse);
        commands.add(heartplunge);
        commands.add(backstab);
        commands.add(enervate);
        commands.add(confuse);
        flags.setConfuse(false);
        return commands;
    }

    private Queue<Command> corpse() {
        Queue<Command> commands = new LinkedList<>();
        Command draw = new Command("draw");
        Command processCorpse = new Command("process corpse");
        Command getAll = new Command("get all");
        Command monitor = new Command("monitor");
        Command glance = new Command("glance");
        commands.add(draw);
        commands.add(processCorpse);
        commands.add(getAll);
        commands.add(monitor);
        commands.add(glance);
        flags.setCorpse(false);
        return commands;
    }

    private Queue<Command> healing() {
        log.fine(playerCharacter.toString());
        Queue<Command> commands = new LinkedList<>();
        if (playerCharacter.getStats().getEndorphine() > 0) {
            Command e = new Command("endorphine 5");
            commands.add(e);
        }
        if (playerCharacter.getStats().getBerserk() > 0) {
            Command b = new Command("berserk 0");
            commands.add(b);
        }
        Command m = new Command("monitor");
        flags.setHealing(false);
        commands.add(m);
        return commands;
    }

    public Queue<Command> doLogic() {
        log.fine("should print...");
        flags = playerCharacter.getFlags();
        log.fine(flags.toString());
        Queue<Command> commands = new LinkedList<>();
        log.fine(playerCharacter.getFlags().toString());
        if (playerCharacter.getFlags().isConfuse()) {
            commands.addAll(confuse());
        }
        if (playerCharacter.getFlags().isCorpse()) {
            commands.addAll(corpse());
        }
        if (playerCharacter.getFlags().isHealing()) {
            commands.addAll(healing());
        }
        if (!playerCharacter.getFlags().isLoggedIn()) {
            commands = new LinkedList<>();
        }
        playerCharacter.setFlags(flags);
        return commands;
    }
}

Answer 1

1) (Minor Detail) Don't make PlayerCharacter a singleton. I understand it only occurs once, but it seems overkill to make it a singleton, and making it an enum is even more confusing.

2) PlayerFlags should instead be an EnumMap<Flag, Boolean> where Flag is a simple enum of all the flags. Similarly, PlayerStats should be EnumMap<Stat, Integer>. I would remove everything else from PlayerCharacter; it would just have two getter methods for the EnumMap's.

3) (Minor Detail) I don't see why you would need the constructor public PlayerStats(List<Entry> stringEntries). You should just create the stats initially with their default start values and they would just be updated as the game goes. Anyway, since this should be an EnumMap, it is not an issue.

5) (Minor Detail) In PlayerLogic, you have members playerCharacter and flags. You initially define flags as something else then the flags member in playerCharacter, but they you do set it to the flags in playerCharacter. That is dangerous. You should not define a separate flags member but manipulate the flags in playerCharacter. Actually, you do playerCharacter.setFlags(flags) at the end of PlayerLogic.doLogic(), so I assume there are some bugs in your handling of the flags. Anyway, if you follow what I write below, you won't even have that method anymore.

6) I would change the name PlayerLogic to CommandProcessor, or similar. It would have one public method to process the incoming command strings.

7) Instead of having Queue<Command> make a subclass of Command called CommandSequence which contains a queue of commands. Command should be an interface with a single method called modify(playerCharacter), or similar. Your methods confuse(), corpse(), healing(), etc. would just be subclasses of Command, or CommandSequence.

8) In CommandProcessor, you will probably need a member Map<String, Command> to process the incoming text (instead of your RegexWorker). Note that the String's in that map would not include any numerical argument, ie. not "endorphine 5", but just the first word, "endorphine".

9) (Minor Detail) For all the stats (hp, cp, adrenaline, ...), you can define a single Command sub-class to deal with all of them:

public BasicStatCommand implements Command {
   private Stat stat;
   private int newValue;

   public BasicStatCommand(Stat stat, int newValue) {
      this.stat = stat;
      this.newValue = newValue;
   }

   @Override
   public void modify(PlayerCharacter player) {
      player.stats.put(stat, newValue);
   }
}

You could also define a BasicStatIncrementCommand where you could add or substract some given value to some stat. You would not to be careful about going below 0; either disallow it, or have some method later in the processing that checks for negative values and takes the appropriate actions (ie, terminate the game if the life value is negative). Also, if all Stat's have a minimum and maximum value, you could encode that information in the enum itself:

enum Stat {
   XP(10, 0, 10),  CP(10, 0, 10), ADRENALINE(0, 0, 10), ...;

   public Stat(int defaultStartValue, int minimum, int maximum) {
      this.defaultStartValue = defaultStartValue;
      this.minimum = minimum;
      this.maximum = maximum;
   }

   private final int defaultStartValue;
   private final int minimum;
   private final int maximum;

   ... add the getters (no setters since those are constants)
}

Summary
So the Command's now contain no reference to the player. Instead the player is just an argument to command.modify(player).