Decoupling and Command Pattern

Question

I am working on some project to learn how to make bigger and better software (multiplayer game) and I found a problem about having in my code a design pattern Command Pattern and anti-pattern God Object. Often I do end up win the latter one and I know that Fascades are okay, but my understanding of something being right and wrong in OOP is very blurry.

I've implemented the Command Pattern. Shortly, my command:

public interface IGameCommand : ICommand
{
    bool Execute(Game game);
}

And an executor

public interface IExecutor<TState, TCommand>
{
    void Execute(TCommand command);
}

Let's say that I have a command that does a lot: modifies some data, plays sound etc.

So, in my case, this should look like this:

public class MagicSpell: IGameCommand
{
    int x; int y; int damage; string soundClipName; string effectName;
    bool Execute(Game game)
    {
         game.gameState.map[x][y].unit.TakeDamage(damage);
         ...
         game.soundPlayer.PlaySound(soundClipName);
         ...
         game.specialEffectPlayer.PlayEffect(effectName);
         ...
    }
}

As you can see, this forces my Game class to become a God object... or doesn't it? Since my Game class contains specialized classes that do their thing I am fulfilling the Single responsibility principle. I have some bad experience with ending up with a God Object, but is my solution viable and acceptable with OOP?

Or maybe is something wrong with my Command Pattern implementation and Execute(Game game)?

Maybe making some specialized commands would help?

Answer 1

1. Commands are simply messengers.

A command is used to transport a "request to do something" from one part of your code to another. It should do very little besides passing on the request when asked to do so. A "magic spell" by itself is not a command, however "casting a spell" could be, and it might look something like this:

class CastSpellCommand : IGameCommand {
  Spell spell;
  Unit target;

  void Execute() {
    spell.Cast(target);
  }
}

Notice how all the command does here is hold on to the necessary objects and invoke an action when executed.

2. Tell, don't ask.

Instead of poking into multiple layers of your Game object, tell the object what you want it to do. In the example above, we tell the Spell to cast on a Unit. We don't know or care that it damages the unit, plays a sound and does a bunch of other things. This might look like:

class Spell {
  int damage;

  void Cast(Unit target) {
    target.TakeDamage(damage);
  }
}

3. Separate domain logic from infrastructure.

Casting a spell and damaging a unit are domain concepts and their implementation is dictated purely by your game's rules. Playing a sound or special effect, on the other hand, is an infrastructure concern that likely depends on some external libraries. You want to separate these concerns since they are likely to change independently. There are many ways to do this; one simple way is to have your domain objects raise events such as SpellCast or UnitDamaged, which your UI layer can listen to and play the corresponding sound or special effect.

Answer 2

Your problem seems to be that your command reaches deep into the innards of other objects, so there is minimal encapsulation. Also, some objects seem to be of technical nature (like gameState, soundPlayer, specialEffectsPlayer).

For object-orientation to make sense, you'll have to pretend objects are living things, and ask them to do things, instead of "you" (i.e. the command) micro-managing everything. Also, objects need to be part of your domain (i.e. your multiplayer game).

For example, there is no such thing as a SoundPlayer in a game. However, there is such a thing as a Sound, Clip or Effect, which can play() themselves. Again, you don't micro-manage, you ask the thing to do its thing.

If you have a lot of dumb objects you will likely end up with god objects too. So if you want to avoid that, you'll have to come up with objects that make sense in your domain, and make them capable of doing their thing on their own.

Answer 3

Or maybe is something wrong with my Command Pattern implementation and Execute(Game game)?

The implementation of the command approach isn't wrong, but you should be implementing it on a much more granular scale (favor many small commands over few big commands).

Let's say that I have a command that does a lot: modifies some data, plays sound etc.

Using what you said as an example, data modification is a backend task, whereas playing a sound is a frontend (UI) task. These are two very different things.

Now, you may be thinking "but it needs to play a sound when this data changes", and that may be the case, but that doesn't mean that changing the data and playing a sound are forever bound together. Instead, you can decouple but chain these actions:

• The command manipulates the data and possible raises a notification that the data was altered.
• The sound player module listens for this notification and independently decides to play a sound when a notification is received.

The end result is the same, but the individual steps (data manipulation, sound playing) were individually evaluated. That's loose coupling.

Since my Game class contains specialized classes that do their thing I am fulfilling the Single responsibility principle.

Coupling and decoupling doesn't quite care about the content of your classes (and their specific responsibility), it cares about how these classes are tied together.

Decoupling and SRP are two separate considerations. While I wouldn't advocate for either, it's technically possible to have loosely coupled god classes (it would be a polytheistic setup though, or you could call them demigods. Whatever floats your godly boat) or, conversely, tightly coupled yet SRP-friendly classes.

Based on the description of your problem, it seems like you're in "tightly coupled SRP-friendly class" territory.

Maybe making some specialized commands would help?

If by "specialized commands" you mean subdividing your current commands into smaller commands (ideally down to the atomic level), then you are right on the money.

Always keep in mind that business requirements might change. By separating the "data manipulation" and "sound playing" commands, you ensure that future you is able to change how/if they are chained together, without having to separate and thus rewrite your single "manipulate data + play sound" command.

Loose coupling mainly promotes flexibility for future changes. It doesn't actually improve a currently correctly implemented codebase that will never be changed in the future.

Promoting loose coupling as good practice relies on the underlying assumption that the odds of needing to change something in the future (for whatever reason, e.g. because you made a mistake or because the requirements got changed on you) are very high and that the cost of making changes to a tightly coupled codebase (and the related risk of generating bugs) are too high.