Large class with single responsibility

Question

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)

Answer 1

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.

Answer 2

You can always use a more abstract definition of a "responsibility." That's not a very good way to judge these situations, at least until you have a lot of experience at it. Notice you easily made four bullet points, which I would call a better starting point for your class granularity. If you are truly following the SRP, it's difficult to make bullet points like that.

Another way is to look at your class members and split off based on methods which actually use them. For example, make one class out of all the methods that actually use self.timer, another class out of all the methods that actually use self.outgoing, and another class out of the remainder. Make another class out of your methods that take a db reference as an argument. When your classes are too big, there are often groupings like this.

Don't be afraid to split it down smaller than you think is reasonable as an experiment. That's what version control is for. The right balance point is much easier to see after taking it too far.

Answer 3

The definition of "responsibility" is notoriously vague, but it becomes slightly less vague if you think of it as a "reason to change". Still vague, but something you can analyze a bit more directly. Reasons for change depend on your domain and how your software will be used, but games are nice example cases because you can make reasonable assumptions about that. In your code, I count five different responsibilities in the first five lines:

self.game = None
self.health = 1000
self.successful_attacks = 0
self.points = 0
self.timer = Timer()

Your implementation will change if the game requirements change in any of these ways:

1. The notion of what constitutes a "game" changes. This may be the least likely.
2. How you measure and track health points changes
3. Your attack system changes
4. Your point system changes
5. Your timing system changes

You're loading from databases, resolving attacks, linking with games, timing things; it seems to me the list of responsibilities is very long already, and we've only seen a small part of your Character class. So the answer to one part of your question is no: your class almost certainly does not follow SRP.

However, I would say there are cases where it's acceptable under SRP to have a 2,500-line class, or longer. Some examples could be:

• A highly complex but well-defined mathematical calculation that takes well-defined input and returns well-defined output. This could be highly optimized code that needs thousands of lines. Proven mathematical methods for well-defined calculations don't have many reasons to change.
• A class that acts as a data store, such as a class that just has yield return <N> for the first 10,000 prime numbers, or the top 10,000 most common English words. There are possible reasons why this implementation would be preferred over pulling from a data store or text file. These classes have very few reasons to change (e.g. you find you need more than 10,000).

Answer 4

Whenever you work against some other entity you could introduce a third object which does the handling instead.

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)

Here you could introduce a 'AttackResolver' or something along those lines that handles the dispatching and gathering of statistics. Is the on_attack here only about character state is it doing more?

You can also revisit state and ask yourself if some of the state you have really needs to be on the character. 'successful_attack' sounds like something you could potentially track on some other class also.