Monday, March 15, 2010

CardFactory - How to create 2,700 different objects?

The CardFactory class is responsible for only one thing, to create all new Card objects. Every Magic card is represented internally as a Card object. The difficultly is that Forge implements 2,700 cards, so that means that CardFactory has to return 2,700 different objects.

So the question becomes how do you actually create a method that can return (output) 2,700 different things? First, I try to refer to the gang of four "Design Patterns" book as often as possible. It mentions the prototype pattern but is seems very vague as to the actual implementation. From page 121, "The hardest part of the Prototype pattern is implementing the Clone operation correctly." So basically you have four very smart people saying that copying an object is hard, yeah.

My very "dumb" solution was to stick all of the objects together in one huge method. The reason why I did this instead of reading them from XML or a text file is because each object was fundamentally different (they were anonymous classes). I couldn't just read the information from a file because each Card object is fundamentally different, Shock is different than Elvish Piper.

I enjoyed having all of the card code in CardFactory because I could quickly cut-and-paste, which is a horrible practice but it helped things zip along. Now things don't "zip" because the Forge architecture isn't very flexible since everything was designed only for basic, simple Magic cards. (Even basic, simple cards are hard to program.)

The one CardFactory method was around 18,000 lines long, which is insane I know but each card had separate brackets and didn't share any variables. I submitted it to the Guinness Book of World Records as the "world's longest method" but they didn't think it was very important. (They were right but I still wanted my plaque, ha.) Currently CardFactory has been divided up into about 5 different classes, which helps the "long method problem" but it doesn't solve it.

Alternatively, I could have created 2,700 different files to hold each card.

I really want to create cards like this.
Card card = new Card("Wrath of God");

SpellAbility spell = new Spell(card)
spell.addResolve("Destroy all creatures");

Hopefully the AI won't need specific instructions on how to handle each card. Right now the computer will play Wrath of God if it has less than 7 life or if it would destroy 2 more of its opponents creatures versus its own creatures. This is just a rough guess when the AI should play Wrath of God and it doesn't work for all situations. This is also a good explanation of why the AI seems so stupid sometimes. (At least AI isn't stupid 100% of the time, ha.)

This is current (ugly) way.
if(cardName.equals("Wrath of God") || cardName.equals("Damnation"))
SpellAbility spell = new Spell(card)
public void resolve()
CardList all = new CardList();

for(int i = 0; i < all.size(); i++)
Card c = all.get(i);

//should the AI play this spell or ability?
public boolean canPlayAI()
CardList human = new CardList(AllZone.Human_Play.getCards());
CardList computer = new CardList(AllZone.Computer_Play.getCards());

human = human.getType("Creature");
computer = computer.getType("Creature");

//the computer will at least
//destroy 2 more human creatures
return computer.size() < human.size()-1 ||
(AllZone.Computer_Life.getLife() < 7 &&



p.s. I don't usually have shoutouts because they are stupid. But here it is anyways, shoutouts to pheadbaq.


wololo said...

CardFactory is by far the ugliest code in MTGForge, and I sure hope someone refactors it one day. This thousands line method is an insult to OO development.
The abilities should be classes, and you should have a hashmap that associates each card name to a constructor, or something like that. It would be waaaay cleaner.

Shock and Elvish Piper are not the same, but Shock is the same as lightning bolt (with different parameters) and Elvish Piper is roughly the same as any other card that moves a target card from one zone to another, such as, maybe, lord of the undead.

there are basic actions in MTG, which should be the base for your abilities: Damage, move a card from one zone to another, add a counter, destroy, tap, untap, etc...
The rest should just be inheritance. It's easier to say than to do, but Wagic handles 5000 cards with 6000 lines of code, and I'm sure Forge could handle the same kind of thing with the amazing work you guys have done recently regarding generic keywords.

There are lots of cool stuff in Forge, but CardFactory is not one of them.

Silly Freak said...

the real difficulty is that a card can be changed to anything else by for example a copy effect. you can't say ElvishPiper extends Creature or something, because elvish piper could become an enchantment. inheritance is not the way to go for card types, unfortunately

Anonymous said...

There is more than one way to use inheritance. For example, in Mox, every card is a Card object (no inheritance here) but

- Every card is created by a different factory (which are derived from a base card factory). This allows me to avoid the "giant card factory class problem".
- Also, I use inheritance a lot for abilities since there is a lot of commonalities between abilities of different cards (as wololo mentioned).

nantuko84 said...

And I like what Silly Freak says most of all

Silly Freak said...

> - Every card is created by a different factory
> (which are derived from a base card factory).
> This allows me to avoid the "giant card factory
> class problem".
> - Also, I use inheritance a lot for abilities
> since there is a lot of commonalities between
> abilities of different cards (as wololo
> mentioned).

I don't think you can really count factories. if the card uses composition to get the needed attributes, and it's created by a factory, inheritance in the factory is merely "writing code more smoothly", but that inheritance isn't something structural to the architecture.

abilities are for sure a better example, they're not as variable as cards ;)

Forge said...

--wololo "Wagic handles 5000 cards with 6000 lines of code"

That is very impressive.

--CardFactory makes me laugh because it is so ridicules but comedy is in the eye of the beholder. :*)

A better way would be to refactor CardFactory so that CardFactory "gets" everything from other class, CardFactory would only "get this resolve" and "get this target". CardFactory has alot of redundant code so it would be good to put the redundant code somewhere else and have CardFactory just "get" it.

Some of the “functionality of Magic” is in CardFactory such as protection which should really be handled elsewhere, so CardFactory is doing more work that it should be.

DennisBergkamp said...

Wow, that's impressive indeed... I didn't even know Wagic had that many cards!

pheedbaq said...

Hey Forge, what's up! Shout out back at ya :)