Is this code evil?

Newbie & Debugging Questions
#1

I am toying with a game (or maybe just a simulator) that involves a lot of data. I need to define lots of characters, skills, creatures and so on. I suppose ideally I should use XML or JSON to store the data but I’m lazy so I am creating little subclasses like this:

public static class FightingBow extends Skill {
    private FightingBow() {
        name = this.getClass().getName();
        experienceMultiplier = 2;
        skillCost = SkillCost.linear;
        lock = new BowTraining();
        description = "General skill in using bows.";
    }

And I have one big class called Skills containing lots of the above inner class definitions.

So I am effectively turning Java into a data file format. Of course this does not lock me in to compiled code, down the road I can add a file reader to load the data from XML. And some regular expressions will convert my Java data file to XML for me - but I was wondering if this approach is in general evil for reasons I cannot see, or virtuous?

#2

If you want to make your game moddable then yes, this is evil. Otherwise, I would still use external files to store data like this - this does not have to be xml files, personally I use classic txt files formated like this:

Key=Value
Another_key=Another_value

Such files are really, really easy to write, modify and read.

#3

instead of what you are doing use enums, they are the same thing you did by hand.


enum Skill
{
  FightingBow(2, SkillCost.linear, new BowTraining, "General skill in using bows."),
  FireMagic(1, linear, new HogwartsEducation(), "burn your enemies with fire balls!!")

  Skills(int e, SkillCost c, Lock l, String d){
    expMult = e;
    cost = c;
    lock = l;
    description = d;
  }
  final int expMult;
  final SkillCost cost;
  final Lock lock;
  final String description;
}

...

Skill skill = Skill.FireMagic

println(skill.name());//this is a method which is provided by Enum for you

I wouldn’t create new file formats anymore, with libs like Jackson json serialisation is soo much easier.

example:


class Skill
{
  String name, description;
  int expMult;
}

...
Skill value = mapper.readValue(new File("BowFighting.json"), Skill .class);

...
BowFighting.json:
{
  "name" : "BowFighting"
  "description" : "..."
  "expMult" : 2
}
#4

I think my approach is not evil, after some consideration. I have plenty of experience with XML and JSON, so I will use those formats down the road (if I need to) but doing it this way means that differences between my data and my model are compile-time errors, not obscure run-time errors. Refactoring to ‘proper’ JSON or XML is trivial after all.

#5

@Danny02
On enum use toString() instead name().

#6

How about something based on the builder pattern instead? This would lead to code like below, without lots of specific subclasses. Makes this potentially easier to customize or switch to an XML / JSON approach later, while keeping your code the same.


Skill fightingBow = Skill.builder()
    .name("Fighting Bow")
    .experienceMultiplier(2)
    .skillCost(SkillCost.linear)
    .lock( new BowTraining() )
    .description("General skill in using bows.")
    .build();

I really like the way they’re doing this in JavaFX to provide a clean Java DSL that also translates easily to XML.

PS. Yes, I think your code is evil! ;D

#7

…or just use a big fat constructor :slight_smile:

public static final Species HUMAN = new Species(70, 3, 60, null, null, "Human");
public static final Species HALFLING = new Species(111, 4, 40, null, null, "Halfling");

Much more compact. It’s not self-documenting, but I can live with that.

#8

no name is just fine, as it does exactly what he wants to do

@ags1
pls take a look at enums, as it is exactly what you try to emulate

#9

@ags1: constructors and methods with more than ~3 parameters are… horrible, it makes each and every callsite a big mess.

#10

Hi there.

How about Valve Key/Value Files?
They are easy to read/write/handle.

  • Longor1996
#11

Actually I agree with you, but building up a big file of constructor calls is not so bad - each call is the same and IntelliJ tells me what all the parameters mean. A more verbose format like XML or JSON will be clearer in the long run, and I can easily migrate to that later on. Once the data has a structure, changing to another structure is trivial.

At work I am always fighting a battle against the monster constructors with a dozen arguments (half of which can be null of course) as I have to explain them to our customers.

#12

Your options are:

  1. Creating separate classes for each different type.
  2. Big constructor.
  3. Builder pattern.
  4. External resources.

Use the builder pattern if you only need to change some of the data. (ie: lots of it is the same as default)
Use External resources if you want modding compatibility/tweaking variable ingame.
Use Big constructors if lots of data is different with every instance.
Use Separate classes when you need to use methods too etc.

#13

NO

#14

The forgotten option: code is data.

#15

Your argument would appear to be backwards! :slight_smile:

Use the builder pattern if lots of data is different with every instance (particularly if that data needs to be known at construction time - eg. immutable state)

Use Big constructors if little data has to be passed in at construction time (ie: lots of it is the same as default, there are few parameters, or you’re happy with mutable state / setters)

evil? ;D

#16

Riven is right. More than 3 params is bad because Eclipse will auto-format your constructor first line into 2 lines. Horrible.

#17

Or turn off all line-wrapping like I did.

#18

That’s more horrible to read D:

#19

To answer you question, no, it isn’t too evil.

Compared to enterprise-y code this is a masterpiece.

#20

Nah… I think my code is definitely evil. My new code structure of ItemContainer<? extends Item> is much more beautiful. When I start loading from files nothing will change for the rest of the app, just the way I build the instances.

I’m not keen on enums (in this case) as they lock you into a closed list and you need to change to a normal class when you change to reading from a file.