CardFactory Refactor?

[frwololo]

Guys, have you considered refactoring the CardFactory* files ?
A quick look at the svn showed me that they represent around 50'000 lines of code.
I know Java is verbose, but I'm sure it could be reduced to 10'000 lines of codes with good inheritance mechanisms and more Object Oriented code...

As an example, the function makeToken appears countless times in the following file:
http://code.google.com/p/cardforge/sour ... tures.java

it could probably be made a static function of the class Ability, that takes arguments and spits a resulting token.

So, in Ability, you would have (pseudocode, haven't done any java in ages):

Code: Select all

static makeToken(Integer p, Integer t, Card card, String imageName, String name, String cost, String[] types) {
Card c = new Card();
             c.setOwner(card.getController());
             c.setController(card.getController());

             c.setImageName(imageName);
             c.setName(name);
             c.setManaCost(cost);
             c.setToken(true);
            
             c.addType("Creature");
                  for (int i < types.length) {
                 c.addType(types[i]);
                  }
             c.setBaseAttack(p);
             c.setBaseDefense(t);

             PlayerZone play = AllZone.getZone(Constant.Zone.Play, c.getController());
             play.add(c);
           }//makeToken()


Then you remove ALL the definitions of makeToken in creaturesFactory, and cards such as captain of the Watch call:

makeToken(1, 1, card,"W 1 1 Soldier", "Soldier", "W", ["soldier"]);

I think the image name can also be removed, as it seems to be computed based on cost/p/t/Name
and there you go, around 500 lines of code can be removed, and token generation becomes easy. It also makes debugging way easier...

[nantuko84]

I like the idea. In MagicWars (that initially is based on Forge architecture) I've created several Factories to reduce spell's code.

I'd like toshare you SpellCreateTokenFactory class (that can be easily integrated into Forge) that has

Code: Select all

Card createToken(Card creator, String name, String types, String abilities, String colors, int attack, int defense)
getSpellAbility(final Card card, final String name, String types, final int power, final int toughness, String abilities, final int multiplier)

methods, but it needs refactoring first after comments from Arch

all I can say that such factories make code more readable
I remember I had Cruel Ultimatum that was coded not effeciently with about 200 lines of code
now it looks like:

Code: Select all

       SpellAbility sacrifice = SpellSacrificeCreatureFactory.getSpellAbility(card, 1, TargetType.OPPONENT, VisibilityType.VISIBLE);
       SpellAbility discard = SpellDiscardFactory.getSpellAbility(card, 3, TargetType.OPPONENT);
       SpellAbility lose5life = SpellLoseLifeFactory.getSpellAbility(card, 5, TargetType.OPPONENT);
       SpellAbility draw3cards = SpellDrawFactory.getSpellAbility(card, 3, TargetType.CONTROLLER);
       SpellAbility returnCreature = SpellReturnFromGraveFactory.getSpellAbility(card, new ChoiceCommand(){
          public void execute() {
            CardList creatures = GameManager.getGraveyard().getPersonalCards(card.getControllerID());
            creatures = creatures.getType("Creature");
            setInputChoice(creatures);
         }
       });
       SpellAbility gain5life = SpellGainLifeFactory.getSpellAbility(card, 5, TargetType.CONTROLLER);
       SpellAbility spell = sacrifice.addChain(discard).addChain(lose5life).addChain(returnCreature).addChain(draw3cards).addChain(gain5life);
       card.addSpellAbility(spell);

so it is good direction to move to

[frwololo]

No comments from the devs of forge? Does it sound like a silly idea? I have very little java experience (and no time to do the actual cleanup), but enough programming experience to spot such obvious flaws, so I guess what I'm asking is if I should look for more of those, or if it's a waste of everyone's time?

[Chris H.]

I do not think that it sounds like a silly idea. Rob and Dennis have spent a lot of time developing the keyword concept and have had a lot of success so far. This helps to reduce the amount of code duplication. I recently converted several of the coded cards to keywords and commented out the separate code objects that are no longer needed. I have found several more that need to be changed over, but it takes time to find and complete these conversions. Rome was not built in a day.

Moving the project to a SVN has helped. It will take some time for us all to get used to this, but it does reduce the amount of time that Dennis has to devote to merging other people's work product. I think that we are moving towards your stated goal, it just takes time. Dennis' work on dividing up the single factory file into multiple factories has helped.

[DennisBergkamp]

It's not a silly idea at all, and I agree there are a lot of obvious flaws and loads of redundant code. But like Chris said, it's a process and it will take some time.
I personally have been focusing more on fixing bugs, and adding new stuff because I get bored otherwise. But you are right, it's probably a good thing to spend some more attention on.

EDIT: I had to run, so I continue here...

Basically it's a matter of priorities. There's a lot of bad AI in the game too, that's something I'd like to make some improvements to as well... but I still think fixing bugs is more important, since there are still some glaring ones in the game that can directly ruin the gameplay. Time's limited
The "token generator" function you (and Nantuko) mentioned would be a great start though, and it would eliminate a lot of lines of redundant code.

[zerker2000]

Certain "devs"(i.e. me) seem to have missed this post. AddChain certainly sounds like a good usable idea, especially since it could be made even simpler with chain links consisting of just inputs or just actions (e.g. addChain(tgtPlayerBad).addChain(dmgTgt5)).

[DennisBergkamp]

Yeah, I noticed that too. Cool stuff, Nantuko

[Rob Cashwalker]

There's a lot that can be done to reduce the redundant code. While it's good to reduce the few simple lines to produce a token into a factory function, it's just as good if not better to make the whole token making process into a keyword-based operation. This approach simultaneously reduces existing code and opens up for new cards without any additional code.

[frwololo]

True, but the two approaches are not incompatible.

In Wagic, generating a token is usually handled by a "keyword-based operation", but obviously the class handling this also allows us to hardcode some token generating methods when the text-based system is not enough.

http://code.google.com/p/wagic/source/b ... ties.h#588
(note that all Abilities in Wagic inherit from "ActivatedAbility", but the same classes are used for non activated abilities, including spells such as sorceries that generate tokens)