Object-Oriented Design – Managing a Large Class with Single Responsibility

class-designobject-orientedsingle-responsibility

I have a 2500 line Character class that:

  • Tracks the internal state of the character in the game.
  • Loads and persists that state.
  • Handles ~30 incoming commands (usually = forwards them to the Game, but some read-only commands are responded to immediately).
  • Receives ~80 calls from Game regarding actions it is takes and relevant actions of others.

It seems to me that Character has a single responsibility: to manage the state of the character, mediating between incoming commands and the Game.

There are a few other responsibilities that have already been broken out:

  • Character has an Outgoing which it calls into to generate outgoing updates for the client application.
  • Character has a Timer which tracks when it is next allowed to do something. Incoming commands are validated against this.

So my question is, is it acceptable to have such a large class under SRP and similar principles? Are there any best practices for making it less cumbersome (eg. maybe splitting methods into separate files)? Or am I missing something and is there really a good way to split it up? I realize this is quite subjective and would like feedback from others.

Here is a sample:

class Character(object):
    def __init__(self):
        self.game = None
        self.health = 1000
        self.successful_attacks = 0
        self.points = 0
        self.timer = Timer()
        self.outgoing = Outgoing(self)

    def load(self, db, id):
        self.health, self.successful_attacks, self.points = db.load_character_data(id)

    def save(self, db, id):
        db.save_character_data(self, health, self.successful_attacks, self.points)

    def handle_connect_to_game(self, game):
        self.game.connect(self)
        self.game = game
        self.outgoing.send_connect_to_game(game)

    def handle_attack(self, victim, attack_type):
        if time.time() < self.timer.get_next_move_time():
            raise Exception()
        self.game.request_attack(self, victim, attack_type)

    def on_attack(victim, attack_type, points):
        self.points += points
        self.successful_attacks += 1
        self.outgoing.send_attack(self, victim, attack_type)
        self.timer.add_attack(attacker=True)

    def on_miss_attack(victim, attack_type):
        self.missed_attacks += 1
        self.outgoing.send_missed_attack()
        self.timer.add_missed_attack()

    def on_attacked(attacker, attack_type, damage):
        self.start_defenses()
        self.take_damage(damage)
        self.outgoing.send_attack(attacker, self, attack_type)
        self.timer.add_attack(victim=True)

    def on_see_attack(attacker, victim, attack_type):
        self.outgoing.send_attack(attacker, victim, attack_type)
        self.timer.add_attack()


class Outgoing(object):
    def __init__(self, character):
        self.character = character
        self.queue = []

    def send_connect_to_game(game):
        self._queue.append(...)

    def send_attack(self, attacker, victim, attack_type):
        self._queue.append(...)

class Timer(object):
    def get_next_move_time(self):
        return self._next_move_time

    def add_attack(attacker=False, victim=False):
        if attacker:
            self.submit_move()
        self.add_time(ATTACK_TIME)
        if victim:
            self.add_time(ATTACK_VICTIM_TIME)

class Game(object):
    def connect(self, character):
        if not self._accept_character(character):
           raise Exception()
        self.character_manager.add(character)

    def request_attack(character, victim, attack_type):
        if victim.has_immunity(attack_type):
            character.on_miss_attack(victim, attack_type)
        else:
            points = self._calculate_points(character, victim, attack_type)
            damage = self._calculate_damage(character, victim, attack_type)
            character.on_attack(victim, attack_type, points)
            victim.on_attacked(character, attack_type, damage)
            for other in self.character_manager.get_observers(victim):
                other.on_see_attack(character, victim, attack_type)

Best Answer

In my attempts to apply SRP to a problem I generally find that a good way to stick to a Single-responsibility-per-class is to choose class names which allude to their responsibility, because it often helps to think more clearly about whether some function really 'belongs' in that class.

Furthermore, I feel that simple nouns such as Character (or Employee, Person, Car, Animal, etc.) often make very poor class names because they really describe the entities (data) in your application, and when treated as classes it's often too easy to end up with something very bloated.

I find that 'good' class names tend to be labels which meaningfully convey some aspect of your program's behaviour - i.e. when another programmer sees the name of your class they already gain a basic idea about that class' behaviour/functionality.

As a rule of thumb, I tend to think of Entities as data models, and Classes as representatives of behaviour. (Although of course most programming languages use a class keyword for both, but the idea of keeping 'plain' entities separated from application behaviour is language-neutral)

Given the breakdown of the various responsibilities you've mentioned for your character class, I would start to lean towards classes whose names are based on the requirement that they fulfil. For example:

  • Consider a CharacterModel entity which has no behaviour and simply maintains the state of your Characters (holds data).
  • For persistence/IO, consider names such as CharacterReader and CharacterWriter (or maybe a CharacterRepository/CharacterSerialiser/etc).
  • Think about what kind of patterns exist between your commands; if you have 30 commands then you potentially have 30 separate responsibilities; some of which may overlap, but they seem like a good candidate for separation.
  • Consider whether you can apply the same refactoring to your Actions as well - again, 80 actions might suggest as many as 80 separate responsibilities, also possibly with some overlap.
  • The separation of commands and actions might also lead to another class which is responsible for running/firing those commands/actions; maybe some kind of CommandBroker or ActionBroker which behaves like your application's "middleware" sending/receiving/executing those commands and actions between different objects

Also remember that not everything related to behaviour necessarily needs to exist as part of a class; for example, you might consider using a map/dictionary of function pointers/delegates/closures to encapsulate your actions/commands rather than writing dozens of stateless single-method classes.

It's fairly common to see 'command pattern' solutions without writing any classes which are built using static methods sharing a signature/interface:

 void AttackAction(CharacterModel) { ... }
 void ReloadAction(CharacterModel) { ... }
 void RunAction(CharacterModel) { ... }
 void DuckAction(CharacterModel) { ... }
 // etc.

Lastly, there are no hard and fast rules as to how far you should go to achieve single responsibility. Complexity for complexity's sake isn't a good thing, but megalithic classes tend to be fairly complex in themselves. A key goal of SRP and indeed the other SOLID principles is to provide structure, consistency, and make code more maintainable - which often results in something simpler.

Related Topic