snippetjavaModerate
How to Train Your Dragon
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:
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 +
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:
You see, why does a Dragon need an enum field to re-state that it is a "Dragon".
This is error-prone:
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
The field names seem unnecessarily shortened.
For example
and immediately more natural than "atk" and "def".
To be precise, a dragon is born... from a Dragon.Builder?
That's interesting :P
This, used in both
A simpler way would be to name the enum constants already Capitalized,
and omit completely the custom
for example:
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:
The
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
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.
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.