Clase grande con responsabilidad única.

13

Tengo una Characterclase de línea 2500 que:

  • Rastrea el estado interno del personaje en el juego.
  • Carga y persiste ese estado.
  • Maneja ~ 30 comandos entrantes (generalmente = los reenvía al Game, pero algunos comandos de solo lectura se responden de inmediato).
  • Recibe ~ 80 llamadas de Gameacciones relacionadas con lo que toma y acciones relevantes de otros.

Me parece que Charactertiene una responsabilidad única: administrar el estado del personaje, mediando entre los comandos entrantes y el Juego.

Hay algunas otras responsabilidades que ya se han desglosado:

  • Charactertiene una Outgoingllamada para generar actualizaciones salientes para la aplicación cliente.
  • Charactertiene un Timerseguimiento que la próxima vez que se le permite hacer algo. Los comandos entrantes se validan contra esto.

Entonces mi pregunta es, ¿es aceptable tener una clase tan grande bajo SRP y principios similares? ¿Existen mejores prácticas para hacerlo menos engorroso (por ejemplo, tal vez dividir los métodos en archivos separados)? ¿O me estoy perdiendo algo y hay realmente una buena manera de dividirlo? Me doy cuenta de que esto es bastante subjetivo y me gustaría recibir comentarios de otros.

Aquí hay una muestra:

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)
usuario35358
fuente
1
Supongo que esto es un error tipográfico: db.save_character_data(self, health, self.successful_attacks, self.points)¿ Querías decir self.healthverdad?
candied_orange
55
Si tu personaje se mantiene en el nivel de abstracción correcto, no veo ningún problema. Si, por otro lado, realmente está manejando todos los detalles de la carga y persistencia, entonces no está cumpliendo con la responsabilidad única. La delegación es realmente clave aquí. Al ver que tu personaje conoce algunos detalles de bajo nivel como el temporizador y demás, tengo la sensación de que ya sabe demasiado.
Philip Stuyck
1
La clase debe operar en un solo nivel de abstracción. No debe entrar en detalles de, por ejemplo, almacenar el estado. Debería poder descomponer trozos más pequeños responsables de los componentes internos. El patrón de comando puede ser útil aquí. Ver también google.pl/url?sa=t&source=web&rct=j&url=http://…
Piotr Gwiazda
Gracias a todos por los comentarios y respuestas. Creo que simplemente no estaba descomponiendo las cosas lo suficiente, y me estaba aferrando a mantener demasiado en las grandes clases nebulosas. Usar el patrón de comando ha sido de gran ayuda hasta ahora. También he estado dividiendo cosas en capas que operan en diferentes niveles de abstracción (por ejemplo, socket, mensajes de juego, comandos de juego). Estoy progresando!
user35358
1
Otra forma de abordar esto es tener "CharacterState" como clase, "CharacterInputHandler" como otra, "CharacterPersistance" como otra ...
T. Sar

Respuestas:

14

En mis intentos de aplicar SRP a un problema, generalmente encuentro que una buena manera de apegarse a una responsabilidad única por clase es elegir nombres de clase que aluden a su responsabilidad, porque a menudo ayuda a pensar más claramente si alguna función realmente 'pertenece' a esa clase.

Por otra parte, siento que los sustantivos simples, tales como Character(o Employee, Person, Car, Animal, etc.) a menudo hacen los nombres de clases muy pobres porque realmente describen las entidades (datos) en su aplicación, y cuando son tratados como clases es a menudo demasiado fácil acabar con Algo muy hinchado.

Creo que los nombres de clase "buenos" tienden a ser etiquetas que transmiten de manera significativa algún aspecto del comportamiento de su programa, es decir, cuando otro programador ve el nombre de su clase, ya obtienen una idea básica sobre el comportamiento / funcionalidad de esa clase.

Como regla general, tiendo a pensar en las Entidades como modelos de datos, y en las Clases como representantes del comportamiento. (Aunque, por supuesto, la mayoría de los lenguajes de programación usan una classpalabra clave para ambos, pero la idea de mantener las entidades 'simples' separadas del comportamiento de la aplicación es neutral en cuanto al lenguaje)

Dado el desglose de las diversas responsabilidades que ha mencionado para su clase de personaje, comenzaría a inclinarme hacia clases cuyos nombres se basan en el requisito que cumplen. Por ejemplo:

  • Considere una CharacterModelentidad que no tiene comportamiento y simplemente mantiene el estado de sus Personajes (contiene datos).
  • Para persistencia / IO, considere nombres como CharacterReadery CharacterWriter (o tal vez un CharacterRepository/ CharacterSerialiser/ etc).
  • Piensa en qué tipo de patrones existen entre tus comandos; si tiene 30 comandos, entonces potencialmente tiene 30 responsabilidades separadas; algunos de los cuales pueden superponerse, pero parecen ser un buen candidato para la separación.
  • Considere si también puede aplicar la misma refactorización a sus acciones; nuevamente, 80 acciones pueden sugerir hasta 80 responsabilidades separadas, también posiblemente con cierta superposición.
  • La separación de comandos y acciones también puede conducir a otra clase responsable de ejecutar / disparar esos comandos / acciones; tal vez algún tipo de CommandBrokero ActionBrokerque se comporta como "middleware" de la aplicación de envío / recepción / ejecutar esos comandos y acciones entre diferentes objetos

También recuerde que no todo lo relacionado con el comportamiento necesariamente debe existir como parte de una clase; por ejemplo, podría considerar usar un mapa / diccionario de punteros / delegados / cierres de funciones para encapsular sus acciones / comandos en lugar de escribir docenas de clases de un solo método sin estado.

Es bastante común ver soluciones de 'patrón de comando' sin escribir ninguna clase que se construya utilizando métodos estáticos que compartan una firma / interfaz:

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

Por último, no existen reglas estrictas y rápidas sobre qué tan lejos debe llegar para lograr la responsabilidad individual. La complejidad por el bien de la complejidad no es algo bueno, pero las clases megalíticas tienden a ser bastante complejas en sí mismas. Un objetivo clave de SRP y, de hecho, de otros principios SOLIDOS es proporcionar estructura, consistencia y hacer que el código sea más fácil de mantener, lo que a menudo resulta en algo más simple.

Ben Cottrell
fuente
Creo que esta respuesta abordó el quid de mi problema, gracias. Lo he estado utilizando para refactorizar partes de mi aplicación y las cosas se ven mucho más limpias hasta ahora.
user35358
1
Debes tener cuidado con los modelos anémicos , es perfectamente aceptable que el modelo de personaje tenga un comportamiento como Walk, Attacky Duck. Lo que no está bien, es tener Savey Load(persistencia). SRP establece que una clase solo debe tener una responsabilidad, pero la responsabilidad del personaje es ser un personaje, no un contenedor de datos.
Chris Wohlert el
1
@ChrisWohlert Esa es la razón del nombre CharacterModel, cuya responsabilidad es ser un contenedor de datos para desacoplar las preocupaciones de la capa de datos de la capa de lógica de negocios. De hecho, aún puede ser deseable Characterque exista una clase de comportamiento en algún lugar también, pero con 80 acciones y 30 comandos, me inclinaría por descomponerlo aún más. La mayoría de las veces encuentro que los sustantivos de entidad son una "pista falsa" para los nombres de clase porque es difícil extrapolar la responsabilidad de un sustantivo de entidad, y es demasiado fácil para ellos convertirse en una especie de navaja suiza.
Ben Cottrell
10

Siempre puede usar una definición más abstracta de una "responsabilidad". Esa no es una muy buena manera de juzgar estas situaciones, al menos hasta que tenga mucha experiencia en ello. Observe que fácilmente hizo cuatro viñetas, lo que yo llamaría un mejor punto de partida para su granularidad de clase. Si realmente está siguiendo el SRP, es difícil hacer viñetas como esa.

Otra forma es mirar a los miembros de su clase y separarse según los métodos que realmente los usan. Por ejemplo, haga una clase de todos los métodos que realmente usan self.timer, otra clase de todos los métodos que realmente usan self.outgoing, y otra clase del resto. Haga otra clase de sus métodos que toman una referencia db como argumento. Cuando sus clases son demasiado grandes, a menudo hay grupos como este.

No tenga miedo de dividirlo más pequeño de lo que cree que es razonable como experimento. Para eso es el control de versiones. El punto de equilibrio correcto es mucho más fácil de ver después de llevarlo demasiado lejos.

Karl Bielefeldt
fuente
3

La definición de "responsabilidad" es notoriamente vaga, pero se vuelve un poco menos vaga si la considera una "razón para cambiar". Todavía vago, pero algo que puedes analizar un poco más directamente. Las razones para el cambio dependen de su dominio y de cómo se usará su software, pero los juegos son buenos ejemplos de casos porque puede hacer suposiciones razonables al respecto. En su código, cuento cinco responsabilidades diferentes en las primeras cinco líneas:

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

Su implementación cambiará si los requisitos del juego cambian de alguna de estas maneras:

  1. La noción de lo que constituye un "juego" cambia. Esto puede ser lo menos probable.
  2. Cómo mide y rastrea los cambios en los puntos de salud
  3. Tu sistema de ataque cambia
  4. Su sistema de puntos cambia
  5. Su sistema de cronometraje cambia

Estás cargando desde bases de datos, resolviendo ataques, enlazando con juegos, cronometrando cosas; Me parece que la lista de responsabilidades ya es muy larga, y solo hemos visto una pequeña parte de su Characterclase. Entonces, la respuesta a una parte de su pregunta es no: es casi seguro que su clase no siga el SRP.

Sin embargo, diría que hay casos en los que es aceptable bajo SRP tener una clase de 2.500 líneas o más. Algunos ejemplos podrían ser:

  • Un cálculo matemático muy complejo pero bien definido que toma datos bien definidos y devuelve resultados bien definidos. Este podría ser un código altamente optimizado que necesita miles de líneas. Los métodos matemáticos probados para cálculos bien definidos no tienen muchas razones para cambiar.
  • Una clase que actúa como un almacén de datos, como una clase que solo tiene yield return <N>los primeros 10,000 números primos, o las 10,000 palabras en inglés más comunes. Hay posibles razones por las que se preferiría esta implementación en lugar de extraer de un almacén de datos o archivo de texto. Estas clases tienen muy pocas razones para cambiar (por ejemplo, si necesita más de 10,000).
Carl Leth
fuente
2

Siempre que trabaje contra alguna otra entidad, podría introducir un tercer objeto que se encarga del manejo.

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)

Aquí puede introducir un 'AttackResolver' o algo por el estilo que maneja el envío y la recopilación de estadísticas. ¿El on_attack aquí solo sobre el estado de los caracteres está haciendo más?

También puede volver a visitar el estado y preguntarse si parte del estado que tiene realmente necesita estar en el personaje. 'exitoso_ataque' suena como algo que potencialmente podría rastrear en alguna otra clase también.

Joppe
fuente