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 anOutgoing
which it calls into to generate outgoing updates for the client application.Character
has aTimer
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
(orEmployee
,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:
CharacterModel
entity which has no behaviour and simply maintains the state of your Characters (holds data).CharacterReader
andCharacterWriter
(or maybe aCharacterRepository
/CharacterSerialiser
/etc).CommandBroker
orActionBroker
which behaves like your application's "middleware" sending/receiving/executing those commands and actions between different objectsAlso 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:
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.