Java RPG battle simulator

Question

I'm coding a turn-based RPG battle simulator (text based) in Java.

I'm doing this as an exercise/project to learn OOP design. I've coded something similar in C, but just porting that code over to java would, of course, be rather silly. I'm going to be taking this extremely slow, focusing on getting each element right and learning as much as I can from it, rather than trying to progress as quick as possible. (I've been programming for about 3 months)

With the above in mind, I'm currently focusing on just:

• Display input prompt
• Get input
• Calculate results
• Display results

So, without any further ado, rip my terrible code to pieces :)

public class Demo
{
    public static void main(String[] args){ 

        Combatant syd = new Combatant("Syd",5,3);           //name,strength,magic
        Combatant goblin = new Combatant("Goblin",5,1);
        Battle encounter = new Battle(syd, goblin);
    }
}

---

package packs.rpg;

public class Combatant{
    // instance variables //    
    private final String name;
    private int maxHealth;
    private int health;
    private int strength;
    private int magic;
    BattleAction battleAction;


    //  constructor(s)  //  
    public Combatant(String name, int strength, int magic){
        this.name = name;
        maxHealth = 10;
        this.health = maxHealth;
        this.strength = strength;
        this.magic = magic;
    }


    //  methods //  
    protected void takeTurn(BattleAction chosenAction, Combatant target){
        if(health > 0){
            setBattleAction(chosenAction);              //strategy pattern
        }
        else{
            setBattleAction(new FaintedAction());
        }
        performAction(target);
    }


    private void performAction(Combatant target){
        battleAction.execute(this, target);
    };

    protected void decreaseHealth(int amount){
        if(health - amount < 0){health = 0;}
        else{health -= amount;}
    }

    protected void increaseHealth(int amount){
        if(health + amount > maxHealth){health = maxHealth;}
        else{health += amount;}
    }       


    //  accessors / mutators    //
    public String getName(){
        return name;
    }

    public int getHealth(){
        return health;
    }   

    public int getStrength(){
        return strength;
    }

    public int getMagic(){
        return magic;
    }

    public void setBattleAction(BattleAction chosenAction){
        battleAction = chosenAction;
    }


}

---

package packs.rpg;

public interface BattleAction{

    public void execute(Combatant user, Combatant target);

    public String toString();
}

class AttackAction implements BattleAction{

    String outcomeText;

    public void execute(Combatant user, Combatant target){
        int damage = BattleHelper.getRng(user.getStrength())+1;     //get randomized damage based on user strength
        target.decreaseHealth(damage);
        outcomeText = (user.getName() + " hits for " + damage + "\n" + target.getName() + "'s health is now " + target.getHealth() + "\n\n");
    }

    public String toString(){
        return outcomeText;     //return the result of the attack (the gnarly string above)
    };
}

class HealAction implements BattleAction{

    public void execute(Combatant user, Combatant target){
        int amount = BattleHelper.getRng(user.getMagic()+1);
        user.increaseHealth(amount);
    }

    public String toString(){
    return "placeholder text for heal";
    };
}

class FaintedAction implements BattleAction{
    String outcomeText;

    public void execute(Combatant user, Combatant target){
        outcomeText =   (user.getName() + " fainted...");
    }

    public String toString(){
        return outcomeText;
    };
}

---

package packs.rpg;
import java.util.*;

public class Battle{        
    private Combatant player;
    private Combatant enemy;
    private BattleAction playerAction;

    public Battle(Combatant thePlayer, Combatant theEnemy){
        player = thePlayer;
        enemy = theEnemy;
        fight();
    }


    private void fight(){
        while(player.getHealth() > 0 && enemy.getHealth() > 0){
            playerAction = BattleHelper.getBattleAction(player);    //poll for input to get choie of action
            player.takeTurn(playerAction, enemy);                       //pass the enemy encase decision was to attack
            BattleHelper.displayTurnOutcome(player.battleAction.toString());    //toString method returns result of chosen action
        }           
    }

}

---

package packs.rpg;
import java.util.*;

class BattleHelper{
    static Random rng = new Random();
    static Scanner in = new Scanner(System.in);


    public static BattleAction getBattleAction(Combatant user){
        System.out.printf("%s's turn\n",user.getName());
        System.out.println("Select an action");
        System.out.println("1. Attack | 2. Heal");
        int i;
        do{
            i = in.nextInt();
        }
        while(i != 1 && i != 2);

        if(i == 1){
            return new AttackAction();
        }
        else{
            return new HealAction();
        }
    }

    public static void displayTurnOutcome(String outcomeText){
        System.out.printf("%s",outcomeText);
    };




    public static int getRng(int range)
    {
        return rng.nextInt(range);
    }
}

I'd love to hear how you'd personally accomplish coding the above (simplified down to my level...) the more approaches I have to compare the better. The only pattern I've looked at in depth thus far is the strategy pattern, and I'm having to stop myself from trying to work it into every situation. Handling I/O without bolting/coupling it directly to the logic is proving difficult for me to get my head around. It's so tempting to just put printf statements right after the event has happened in the method that processed it, but from what I've read, that's a big no-no.

I don't have internet at home (posting from the library) so if I don't reply for 24 hours, that's why. Any questions about how the code works, of course just ask (I hope the formatting/structure is easy enough to follow).

Answer 1

I'd say in general, it is quite decent code. Nothing written too complicated, variables are named so that I understand it and 'stuff' which belongs together is mostly separated within classes.

Some java coding conventions:

• spaces after commas (e.g. Demo, creation of Combatant), in front of opening curly brackets, ...
• interfaces: Do not need public modifier in methods, they are public per se
• else is on the same line as the closing curly bracket
• best you check the code conventions, usually IDE's do have a formatter template (-> always format your code)

Comments

• Comments do not end with '//'.
• Comments such as 'instance variables' or 'methods' are considered "clutter". There are conventions and guidelines on how order your code.

Combatant

• maxHealth is always 10 and never gets changed, so you might want to declare that as a constant.
• The takeTurn method calls performAction. I personally prefer smaller methods over one large and therefore complicated methods, but I also prefer one small method over two small methods. And since performAction is only called by takeTurn, I don't see any reason why not call battleAction within the takeTurn method.
• decreaseHealth / increaseHealth: Formatting ...

Actions

• toString is already a method in the Object class. It is either not needed, if you want to call toString, but I usually prefer a separate method.
• The Actions are displaying text and I don't think that's their job. Just think about not having a console client, or let's say, a window ui and console client and how you'd solve the displaying of data in that case.
• The execute method have two Combatant parameters. But a self-heal doesn't work like that. So you basically have to pass a target in the HealAction, but you don't use it. I think in that case, it is better to pass the needed parameters to the constructor of the actions. And also maybe there will be a 'group attack' action, the interface therefore cannot do that, since it needs n targets.

BattleHelper

• The do while is a bit complicated to understand. I think, first of all, I'd rename i to action. And then, move the reading of said action to a separate method. Then you just have your sysos, something like readAction, and then the if/else
• Instead of if/else, you can do a switch-case, so you end up with a default...
• Also, assume you have 100 possible actions, you will have 100 else-if's. You might want to think about how to solve that problem.

Answer 2

Types

I think I'd start by building a few types to implement behaviors you want. The first that occurs to me is a saturating integer type to store (in your case) a player's health (and probably a few other things like magic level, though that's not immediately clear).

Following this strategy, health would look something like thhis: saturating<int> health(0, 10);. saturating is (obviously) a generic that assures you can't go outside the prescribed limits.

Then I'd probably do another type specifically for health, based on that, that had (for example) a healthy() member, so in Battle::fight, instead of this:

while(player.getHealth() > 0 && enemy.getHealth() > 0){

You'd have something like:

while (player.healthy() && enemy.healthy()) {

Maybe "healthy" isn't quite the right word though--perhaps "alive" or something like that is closer. I'll leave it to you to figure out exactly what it should be based on the rules your game is implementing. The point, however, is that when you're at the level of a player, you should be using a type that expresses actions that are meaningful in terms of a player.

In getBattleAction, I'd also use an enumeration instead of an integer. I suppose some might consider it overkill for only two values, but instead of this:

    if(i == 1){
        return new AttackAction();
    }
    else{
        return new HealAction();
    }

I'd rather see something like:

switch (userAction) { 
    case Heal: // ...
    case Attack: // ...
}

If you continue development on your game, it's almost inevitable that you'll add more actions a user can take, and this is readable in isolation, instead of having to look at the prompt you printed out to the user to get a reasonable verification that 1 means heal and 2 means attack.

Names

That brings up another obvious point: i works perfectly fine (IMO, anyway) for something like a for loop: for (int i=0; i<10; i+++)..., but not for much more than that--in this case, userAction (or something similar) would give a much better indication of the actual meaning of the variable.

Immediacy

I can't help feeling that code like this:

    if(health > 0){
        setBattleAction(chosenAction);              //strategy pattern
    }
    else{
        setBattleAction(new FaintedAction());
    }
    performAction(target);

...mostly obfuscates the intent. It seems to me that under the circumstances, it's simpler to just have something like:

if (health > 0) // if player.healthy()
    performAction(chosenAction);
else
    performAction(new FaintedAction());

...and if possible, I'd prefer the still simpler else faint();

Strunk and White advised us to "omit needless words", and it applies at least as well to code as prose.

Answer 3

Here's my updated code. Some things I've changed/added:

• Moved String building out of BattleAction logic and into a dedicated MessageBuilder class.

• BattleActions now have instance variables (which are passed to the constructor) to hold things like the target and amount of damage to avoid them having to be passed in (as parameters) when the action in executed. They are now more of a self contained unit ready to be executed by the user, or even without a user if necessary (say in the case of poisoning, or some kind of area buff/debuff).

• Seperated "%s hit for %d damage" from "%s health is now.." because although the latter inevitably follows the former, they are still separate statements. I plan on eventually having the Combatant's health values displayed and updated on their own dedicated part of the screen, with no associated messages.

---

package packs.rpg;

public interface BattleAction {

    void execute();

    String getResult(); 
}



class AttackAction implements BattleAction {

    Combatant target;
    public int damage;
    String result;

    AttackAction(Combatant target, int damage, String result) {
        this.target = target;
        this.damage = damage;
        this.result = result;
    }

    public void execute() {
        target.decreaseHealth(damage);
    }

    public String getResult() {
        return result;
    }

}

class NoAction implements BattleAction {

    String result;

    NoAction(String result) {
        this.result = result;
    }

    public void execute(){
   }

    public String getResult() {
        return result;
    }
}

---

package packs.rpg;
import java.util.*;

public class Battle {   
    int input;  
    private Combatant player;
    private Combatant enemy;
    private BattleAction playerAction;

    public Battle(Combatant thePlayer, Combatant theEnemy) {
        player = thePlayer;
        enemy = theEnemy;
        fight();
    }


    private void fight(){
        while(player.getHealth() > 0 && enemy.getHealth() > 0) {
            playerAction = BattleHelper.getBattleAction(player,enemy);  //poll for input to get choice of action
            player.takeTurn(playerAction, enemy);                       //pass the enemy encase decision was to attack
            BattleHelper.displayTurnResult(playerAction, enemy);
        }           
    }

}

---

package packs.rpg;
import java.util.*;

class BattleHelper {
    static Random rng = new Random();
    static Scanner in = new Scanner(System.in);


    public static BattleAction getBattleAction(Combatant user, Combatant target) {
        System.out.printf(MessageBuilder.inputPrompt(user.getName()));
        int action;
        do{
            action = in.nextInt();
        }
        while(action != 1 && action != 2);

        if(action == 1){
            int damage = getRng(user.getStrength())+1;
            return new AttackAction(target, damage, MessageBuilder.attackAction(user.getName(), damage));
        }
        else{
            return new NoAction(MessageBuilder.noAction(user.getName()));
        }
    }

    public static void displayTurnResult(BattleAction action, Combatant target) {
        System.out.printf("%s",action.getResult());
        System.out.printf("%s health is now %d\n\n",target.getName(), target.getHealth());
    }




    public static int getRng(int range) {
        return rng.nextInt(range);
    }
}

---

package packs.rpg;

public class MessageBuilder {

    public static String inputPrompt(String userName) {
        return (userName + "'s turn\nSelect an action\n1. Attack | 2. Do Nothing");
    }

    public static String attackAction(String userName, int damage) {
        return (userName + " hits for " + damage + "\n");
    }

    public static String noAction(String userName) {
        return (userName + " is loafing around...");
    }
}