HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavaModerate

How to Train Your Dragon

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
trainyourdragonhow

Problem

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:

  • 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.



  • 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.



  • 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 +

Solution

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.

Code Snippets

class Dragon {
    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;
}
//You can't generate a dragon. A dragon is born!
    public Dragon born(){ return new Dragon(this); }
@Override
public String toString() {
    return super.toString().substring(0, 1).toUpperCase() + 
        super.toString().substring(1).toLowerCase();
}
public enum Race {
    Dragon
}

Context

StackExchange Code Review Q#77264, answer score: 17

Revisions (0)

No revisions yet.