How to Train Your Dragon

Question

I started out practicing on implementing the builder pattern and somehow ended it up with this 2 hours later. It isn't really much, but it works and I'm hoping review should bring about a lot of insight.

I'm curious in pinpointing:

1. Which part(s) are well executed? I've never done anything 'game' like so I have no idea what's a good idea and what isn't.
2. Which part(s) could be considered poorly executed? I wanted to standardize the outputs and decreased repetition as best I could through method calls but I still get the feeling that a few things are redundant.
3. General feedback on efficiency of the program. Particularly, little things, I'm wondering if it would make more sense to make Stringbuilder and use append instead of using string concatenation.

The Dragon class:

class Dragon {
    int atk, def, hp, hpMax, mp, mpMax, exp, lvl;
    String name;
    Element element;
    Race race = Race.DRAGON;

    Dragon(Builder d) {
        atk = d.atk; def = d.def; hp = d.hp; hpMax = d.hp;
        mp = d.mp; mpMax = d.mp; lvl = d.lvl;
        name = d.name; element = d.element;
    }

    static class Builder {
        int atk = 0, def = 0, hp = 1, mp = 0, lvl = 1;
        String name;
        Element element = Element.NEUTRAL;

        Builder(String name){ this.name = name; }

        public Builder atk(int val){ atk = val; return this; }
        public Builder def(int val){ def = val; return this; }
        public Builder hp(int val){ hp = val; return this; }
        public Builder mp(int val){ mp = val; return this; }
        public Builder element(Element e){ element = e; return this;}

        //You can't generate a dragon. A dragon is born!
        public Dragon born(){ return new Dragon(this); }
    }

    public void addExperience(int exp){ this.exp  += exp; }

    public String getStatus() {
        return this.name + " has " + this.atk + " attack " + this.def +
            " defense " + this.mp + "/" + this.mpMax + " mana and " +
            this.hp + "/" + this.hpMax + " health."
        ;
    }
}

Element enum:

    public enum Element {
    LIGHT, DARK, FIRE, WATER, AIR, EARTH, NEUTRAL;

    @Override
    public String toString() {
        return super.toString().substring(0, 1).toUpperCase() + 
            super.toString().substring(1).toLowerCase();
    }
}

Race enum:

public enum Race {
    DRAGON;

    @Override
    public String toString() {
        return super.toString().substring(0, 1).toUpperCase() + 
            super.toString().substring(1).toLowerCase();
    }
}

I haven't yet really implemented these two yet but I included them for the sake of completion.

Main:

import java.util.Scanner;

public class BattleSim {
    static Scanner input = new Scanner(System.in);

    public static void main(String[] args) {
        System.out.print("What is your name? ");
        String name = input.nextLine();
        Dragon player = new Dragon.Builder(name)
            .atk(5).def(2).hp(25).mp(7).born();
        Dragon challenger = new Dragon.Builder("Glaurung")
            .atk(6).hp(15).born();

        System.out.println("You are a Dragon.\n" +
            "Not the order your underlings around 'big bad' that barely" +
            " ever shows up kind" +
            "but bonafide sky soaring, loot hoarding, city scorching Dragon."
        );
        System.out.println("\nWhile you were out being awesome another dragon " +
            " stole your princess.\nYour damsel's in distress." +
            " You 'gonna take that?"
        );
        battle(player, challenger);

        System.out.println("Thanks for checking this out fellow CRers.");
    }

    public static int printBattleMenu() {
        System.out.print("\nBattle menu\n1: Attack\n2: Defend " +
            "\n3: Abilities\n4: Status\n5: Enemy Status\nEnter a choice: "
        );

        int selection = input.nextInt();

        while (selection > 5 || selection < 1) {
            System.out.print("Out of range. Enter a choice (1-5): ");
            selection = input.nextInt();
        }
        return selection;
    }

    public static int printAbilityMenu() {
        System.out.print("\nAbility menu\n1: Rage Claw - 2 mp" +
            "\n2: Meditate - gain 3 mp\n3: Healing breath - 3 mp" +
            "\n4: Back\nEnter a choice: "
        );

        int selection = input.nextInt();

        while (selection > 5 || selection < 1) {
            System.out.print("Out of range. Enter a choice (1-4): ");
            selection = input.nextInt();
        }
        return selection;
    }

    public static void printBattleStatus(Dragon x, Dragon y) {
        System.out.println(x.name + " has " + Math.max(0, x.hp) +
            " health and the opposing " + y.name + " has " +
            Math.max(0, y.hp) + " health remaining."
        );
    }

    public static void battle(Dragon x, Dragon y){
        int damageDealt, damageTaken, recoveryValue;

        System.out.println("\t\t\tVS. " + y.name);

        do {
            switch(printBattleMenu()) {
                case 1: 
                    damageDealt = Math.max(0, x.atk - y.def);
                    damageTaken = Math.max(0, y.atk - x.def);

                    System.out.println(x.name + " deals " + damageDealt +
                        " damage and takes " + damageTaken  + " damage."
                    );

                    x.hp  -= damageTaken;
                    y.hp  -= damageDealt;

                    // Print current status
                    printBattleStatus(x, y);
                    break;
                case 2:
                    damageTaken = Math.max(0, y.atk - x.def * 2);

                    System.out.println(x.name + " takes a defensive stance and " +
                        " takes" + damageTaken + "damage."
                    );
                    printBattleStatus(x, y);
                    break;
                case 3: 
                    switch(printAbilityMenu()) {
                        case 1:
                            damageTaken = Math.max(0, y.atk - x.def);
                            if (x.mp < 2) {
                                System.out.println("Insufficient mana." +
                                    "\nOpposing " + y.name + " took advantage of your" +
                                    " foolishness to attack."
                                );
                            } else {
                                damageDealt = Math.max(0, x.atk * 2 - y.def);
                                x.mp -= 2;

                                System.out.println(x.name + 
                                    "'s claws are infused with mana!"
                                );

                                System.out.println(x.name + " deals " +
                                    damageDealt + " damage and takes " +
                                    damageTaken + " damage."
                                );
                                y.hp -= damageDealt;
                            }

                            x.hp  -= damageTaken;
                            printBattleStatus(x, y);
                            break;
                        case 2:
                            damageTaken = Math.max(0, y.atk - x.def);
                            System.out.println(x.name +
                                " absorbs the mana of the land."
                            );
                            x.mp = Math.min(x.mpMax, x.mp + 3);

                            System.out.println(x.name + " takes " +
                                damageTaken + " damage."
                            );
                            x.hp  -= damageTaken;

                            printBattleStatus(x, y);
                            break;
                        case 3:
                            damageTaken = Math.max(0, y.atk - x.def);
                            if (x.mp < 3) {
                                System.out.println("Insufficient mana." +
                                    "\nOpposing " + y.name + " took advantage" +
                                    " of your foolishness to attack."
                                );
                            } else {
                                recoveryValue = (int)Math.floor(x.hpMax * 0.3);
                                x.hp = Math.min(x.hpMax , x.hp + recoveryValue);
                                x.mp  -= 3;

                                System.out.println(x.name + " recovers " +
                                    recoveryValue + " health!"
                                );
                            }

                            x.hp  -= damageTaken;
                            System.out.println(x.name + " takes " +
                                damageTaken + " damage."
                            );
                            printBattleStatus(x, y);
                            break;
                        case 4: // do nothing
                            break;
                    } // End of Ability Switch
                    break; 
                case 4: 
                    System.out.println(x.getStatus());
                    break;
                case 5:
                    System.out.println("Opposing " + y.getStatus());
                    break;
            } // End of Selection Switch            
        } while(x.hp > 0 && y.hp > 0);

        if (x.hp == y.hp){
            System.out.println("It is a double K.O.");
        } else if (x.hp > y.hp){
            System.out.println(x.name + " is the victor!");
        } else {
            System.out.println(y.name + " has slain you!");
        }
        // setting values back to full
        x.mp = x.mpMax;
        x.hp = x.hpMax;
    }
}

Answer 1

This is smelly:

class Dragon {
    Race race = Race.DRAGON;

You see, why does a Dragon need an enum field to re-state that it is a "Dragon".

---

This is error-prone:

Dragon(Builder d) {
    atk = d.atk; def = d.def; hp = d.hp; hpMax = d.hp;
    mp = d.mp; mpMax = d.mp; lvl = d.lvl;
    name = d.name; element = d.element;
}

It's pretty hard to read which values from the builder get assigned. It's easy to get lost in the middle of those lines and possibly forget to assign something very important. The common writing style is to have all assignments on their own lines.

It's also good to stop here for a second about naming... Why name a Builder variable "d"? How about builder? The field names seem unnecessarily shortened. For example attack and defense are not that long, and immediately more natural than "atk" and "def".

---

    //You can't generate a dragon. A dragon is born!
    public Dragon born(){ return new Dragon(this); }

To be precise, a dragon is born... from a Dragon.Builder? That's interesting :P

---

This, used in both Element and Race is very tedious:

@Override
public String toString() {
    return super.toString().substring(0, 1).toUpperCase() + 
        super.toString().substring(1).toLowerCase();
}

A simpler way would be to name the enum constants already Capitalized, and omit completely the custom toString implementation, for example:

public enum Race {
    Dragon
}

Or, if you really prefer enum constants as all-caps, at least move the common capitalization logic to a utility class to eliminate duplicated code, for example:

public class StringUtils {
    private StringUtils() {
        // utility class, forbidden constructor
    }

    public static String toCapitalized(String label) {
        return label.substring(0, 1).toUpperCase() + label.substring(1).toLowerCase();
    }
}

public enum Element {
    LIGHT, DARK, FIRE, WATER, AIR, EARTH, NEUTRAL;

    @Override
    public String toString() {
        return StringUtils.toCapitalized(super.toString());
    }
}

---

The BattleSim.battle method is awfully long. It would be good to try to break it into smaller pieces.

---

In terms of OOP, there isn't much to see here. Although you called the main class Dragon, it's not very Dragon-like. It might as well be a Human Wizard: it has attributes like attack, defend, mana, but it doesn't have features like breath attack, paralyzing terror, flying. It's not clear how to extend the existing classes to add such and more features, it seems that thorough thinking would be needed.

Answer 2

1. StringBuilder vs plain String concatenation

I'm wondering if it would make more sense to make Stringbuilder and use append instead of using string concatenation.

My advice is, don't sweat the small stuff. My personal experience is:

• Many concatenation operations that needs to be done incrementally (e.g. across different if branches) and in the fastest way? StringBuilder

• The need to create Strings by substitution within standardized Strings? String.format()

• Everything else? String concatenation

String concatenation should be used for most cases, as the readability is the easiest. Besides, the Java Language Specifications and String's Javadoc mention that plain String concatenation may be converted at compile-time to use a StringBuilder too.

2. battle() long and hard

Like what @janos said, BattleSim.battle() is perhaps better refactored into more manageable pieces. In fact, I'll go further to say that Dragons battle() each other, so why not put it as a method of your Dragon class?

3. The Dragon race

Last I checked, a Dragon is a mythical animal species, not a specific race. It might be better to rename your Race enum, or simply remove this single-entry enumeration altogether.

Answer 3

For the Main.battle() you could refactor a lot:

• Magic numbers for the mana check conditions, recovery and costs in Main.battle() should be constants, improving readability and maintainability.
• Magic numbers in various case statements would be more readable if you mapped them to constants, e.g., ATTACK=1, DEFEND=2, etc.
• The while condition could be an extracted local boolean variable atLeastOneDragonAlive = x.hp > 0 && y.hp > 0;
• dragonX and dragonY are more readable than x and y
• printAbilityMenu() is misleading. Its main function is to get the choice of the player. switch(chooseAbility()) makes more sense to me.
• Dragon exposes its implementation details (int atk, def, hp, hpMax, mp, mpMax, exp, lvl;). Not so great for encapsulation, but perhaps they'll never change and you can live with it. An alternative could be providing methods like Dragon.takeDamage(...) and Dragon.recover(...) that encapsulate rules, making the simulator simpler. It all depends on what changes in the future: if different races have rules about recovering, then your simulator will have a bunch of cases to figure it out. Encapsulating it in each type of monster would simplify the simulator and allow easy changes to adding new monsters if the rules for recovery vary.

EDIT:

• Builder the way you've done it is questionable here. According to my copy of the GoF, Builder's intent is

  Separate the construction of a complex object from its representation so that the same construction process can create different representations.

  What's the problem you're trying to solve with a Builder? To me, it means that in the future you'll be adding some new attributes that may not apply to some dragons, and you want a construction of a Dragon to be flexible. Or, dragons are composed of different things when they're born (that doesn't seem right to me as all dragons have all the attributes you show). A single constructor for your Dragons, even though it might have a bunch of arguments, is probably adequate. I did a combat simulator that had code like hero1 = new Hero("Panos", 13, 11, Weapon.MORNINGSTAR, Armor.LEATHER, Shield.SMALL_SHIELD); Builder is extra complexity compared to a single constructor. The construction process is maybe slightly more readable (my example with a single constructor doesn't show what 13 and 11 are, whereas with a builder it would be strength(13) and dexterity(11)). I'm not sure that advantage is worth all the complexity of a Builder. See more controversy about Builder at https://stackoverflow.com/questions/1638722/how-to-improve-the-builder-pattern -- that said, I think you implemented it fine.