Reputation: 827
I'm writing an application to help my friends run some numbers to balance a game.
This has become a larger project than I initially thought it would be, due to the large number of classes and stat variables.
I'm in the middle of making HeroObject
s which are supposed to represent the stats of the hero. This object is extended by each class (i.e. Marksman
) which provides specific functionality related to the class. I'm reading these heros in via a csv
file so multiple classes and skill formulas can be tested at once. Here's the function I'm using to make the HeroObject
s:
private HeroObject makeHero(String[] heroLine) {
switch(heroLine[11].toLowerCase()) {
case "rogue":
return new Rogue(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "marksman":
return new Marksman(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "knight":
return new Knight(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "lancer":
return new Lancer(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "windadept":
return new WindAdept(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "wateradept":
return new WaterAdept(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "lightningadept":
return new LightningAdept(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "lightadept":
return new LightAdept(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "iceadept":
return new IceAdept(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "fireadept":
return new FireAdept(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "earthadept":
return new EarthAdept(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
case "darkadept":
return new DarkAdept(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
}
return null;
}
I don't have to point out how much of this is repetitive code, but I'm unsure how I can break the case section into its own class without checking what the class is again.
tl;dr:
Each class is extending HeroObject
and thus has the same constructor but I still need these to be created via their respective interfaces. How can I functionally separate the following blocks:
return new SomeClass(Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9]),
heroLine[10]);
Upvotes: 1
Views: 66
Reputation: 1
Try making another HeroObject Constructor but have it take in an Array of ints and the last value.
public HeroObject (int[] heroValues, Object heroLine )
{
this(heroValues[0], heroValues[1],
heroValues[2], heroValues[3],
heroValues[4], heroValues[5],
heroValues[6], heroValues[7],
heroValues[8], heroValues[9],
heroLine);
}
Build the int array before the switch.
int[] heroValues = new int[] {
Integer.valueOf(heroLine[0]), Integer.valueOf(heroLine[1]),
Integer.valueOf(heroLine[2]), Integer.valueOf(heroLine[3]),
Integer.valueOf(heroLine[4]), Integer.valueOf(heroLine[5]),
Integer.valueOf(heroLine[6]), Integer.valueOf(heroLine[7]),
Integer.valueOf(heroLine[8]), Integer.valueOf(heroLine[9])};
Then call the constructor like so.
new SomeClass(heroValues, heroLine[10]);
Upvotes: 0
Reputation: 308938
You should certainly separate reading the Excel file and creating instances from the Hero
class.
I'd create a HeroFactory
(aka virtual constructor) that could give me instances of Hero
and any other subclass.
It looks like the columns in the Excel file are the same for each case. Instead of a switch statement, pass in the type of object you want.
public class HeroFactory {
public static final Hero createHero(Class heroClass) {
// create the type of Hero here based on Class passed in. You can use reflection to make it clean. No switch needed.
}
}
It'd be easy to cut down on the amount of code with another constructor:
public class Hero {
public Hero(String [] parameters) {
// initialize your stuff by iterating over the parameters instead of passing 11 values.
}
}
Upvotes: 5
Reputation: 140525
On the one side, you are on a good path: you are (somehow) turning towards a factory pattern. Of course, as duffymo says, this could be extended.
But that won't solve your real problem: and that is - you are using low-level abstractions! You see, you are using an array of strings to represent important information; and thus your factory code is utterly complicated.
Meaning: in well-designed OO setting, you should avoid objects that need more than 2, 3 parameters upon creation. In other words: you really have to step back and figure how to improve the quality of the model you are basing your design on. You see, classes/interfaces, they all exist to provide useful abstractions. Your classes fail to do that; because in essence, you are switching on strings; and building everything from strings.
The underlying reason for that: probably CSV is a bad format for data that contains META information?
Long story short: you are trying to make your code work with data that carries insufficient meta information. If that is an option, you should step back and consider if you could change from CSV to JSON for example.
Meaning: don't put lines of strings into files. Put objects into your file; and read them back from there.
If all of that is not possible; then there are only two choices in my eyes: as duffymo suggests; consider using reflections. But ... doing such "dynamic" things ... that is not something that Java is really good at. If you really need a lot of "code dynamics"; based on the data you are working with - maybe other languages would get you there faster. You know, meta programming is much easier to do in python, ruby, ...
Upvotes: 2