Classe grande com responsabilidade única

13

Eu tenho uma Characterclasse de linha 2500 que:

  • Rastreia o estado interno do personagem no jogo.
  • Carrega e persiste nesse estado.
  • Manipula ~ 30 comandos recebidos (geralmente = os encaminha para o Game, mas alguns comandos somente leitura são respondidos imediatamente).
  • Recebe ~ 80 chamadas de Gameações a serem tomadas e ações relevantes de outras pessoas.

Parece-me que Charactertem uma única responsabilidade: gerenciar o estado do personagem, mediando entre os comandos recebidos e o Jogo.

Existem algumas outras responsabilidades que já foram divididas:

  • Characterpossui uma Outgoingchamada para gerar atualizações de saída para o aplicativo cliente.
  • Charactertem um Timerque rastreia quando é permitido fazer algo a seguir. Os comandos de entrada são validados com relação a isso.

Então, minha pergunta é: é aceitável ter uma classe tão grande sob SRP e princípios semelhantes? Existem práticas recomendadas para torná-lo menos complicado (por exemplo, talvez dividir métodos em arquivos separados)? Ou estou faltando alguma coisa e existe realmente uma boa maneira de dividir? Sei que isso é bastante subjetivo e gostaria de receber feedback de outras pessoas.

Aqui está uma amostra:

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)
user35358
fonte
1
Presumo que seja um erro de digitação: db.save_character_data(self, health, self.successful_attacks, self.points)você quis dizer self.healthcerto?
candied_orange
5
Se o seu personagem permanecer no nível correto de abstração, não vejo problema. Se, por outro lado, ele realmente está lidando com todos os detalhes do carregamento e persistência, então você não está seguindo uma única responsabilidade. Delegação é realmente a chave aqui. Vendo que seu personagem conhece alguns detalhes de baixo nível, como o cronômetro e outros, tenho a sensação de que ele já sabe demais.
Philip Stuyck 27/01
1
A classe deve operar em um único nível de abstração. Não deve entrar em detalhes como, por exemplo, armazenar o estado. Você deve ser capaz de decompor pedaços menores responsáveis ​​por componentes internos. O padrão de comando pode ser útil aqui. Veja também google.pl/url?sa=t&source=web&rct=j&url=http://…
Piotr Gwiazda
Obrigado a todos pelos comentários e respostas. Eu acho que simplesmente não estava decompondo as coisas o suficiente e estava me apegando a manter muito em grandes aulas nebulosas. O uso do padrão de comando tem sido uma grande ajuda até agora. Também estive dividindo as coisas em camadas que operam em diferentes níveis de abstração (por exemplo, socket, mensagens de jogos, comandos de jogos). Estou progredindo!
precisa saber é o seguinte
1
Outra maneira de resolver isso é ter "CharacterState" como uma classe, "CharacterInputHandler" como outra, "CharacterPersistance" como outra ...
T. Sar - Restabelecer Monica

Respostas:

14

Nas minhas tentativas de aplicar SRP a um problema, geralmente considero que uma boa maneira de manter uma responsabilidade única por classe é escolher nomes de classe que aludam a sua responsabilidade, porque geralmente ajuda a pensar com mais clareza se alguma função realmente 'pertence' a essa classe.

Além disso, sinto que os nomes simples, como Character(ou Employee, Person, Car, Animal, etc.) muitas vezes fazem nomes de classes muito pobres, porque eles realmente descrever as entidades (dados) em seu aplicativo, e quando tratadas como classes muitas vezes é muito fácil de acabar com algo muito inchado.

Acho que nomes de classe 'bons' tendem a ser rótulos que transmitem significativamente algum aspecto do comportamento do seu programa - ou seja, quando outro programador vê o nome da sua classe, ele já obtém uma idéia básica sobre o comportamento / funcionalidade dessa classe.

Como regra geral, costumo pensar em Entidades como modelos de dados e em Classes como representantes de comportamento. (Embora, é claro, a maioria das linguagens de programação use uma classpalavra - chave para ambos, mas a idéia de manter entidades 'simples' separadas do comportamento do aplicativo é neutra em termos de linguagem)

Dada a repartição das várias responsabilidades que você mencionou para sua classe de personagem, eu começaria a me concentrar em classes cujos nomes se baseiam no requisito que eles cumprem. Por exemplo:

  • Considere uma CharacterModelentidade que não tem comportamento e simplesmente mantém o estado de seus Personagens (mantém dados).
  • Para persistência / E / S, considere nomes como CharacterReadere CharacterWriter (ou talvez um CharacterRepository/ CharacterSerialiser/ etc).
  • Pense em que tipo de padrões existem entre seus comandos; se você possui 30 comandos, potencialmente possui 30 responsabilidades separadas; alguns dos quais podem se sobrepor, mas parecem ser um bom candidato à separação.
  • Considere se você também pode aplicar a mesma refatoração às suas ações - novamente, 80 ações podem sugerir até 80 responsabilidades separadas, também possivelmente com alguma sobreposição.
  • A separação de comandos e ações também pode levar a outra classe responsável por executar / disparar esses comandos / ações; talvez algum tipo CommandBrokerou ActionBrokerque se comporte como o "middleware" do seu aplicativo enviando / recebendo / executando esses comandos e ações entre objetos diferentes

Lembre-se também de que nem tudo relacionado ao comportamento precisa necessariamente existir como parte de uma classe; por exemplo, você pode considerar usar um mapa / dicionário de ponteiros / delegados / fechamentos de função para encapsular suas ações / comandos, em vez de escrever dezenas de classes de método único sem estado.

É bastante comum ver soluções de 'padrão de comando' sem escrever nenhuma classe criada usando métodos estáticos que compartilham uma assinatura / interface:

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

Por fim, não existem regras rígidas e rápidas sobre o quão longe você deve ir para alcançar uma responsabilidade única. Complexidade por complexidade não é uma coisa boa, mas as classes megalíticas tendem a ser bastante complexas em si mesmas. Um objetivo principal do SRP e, de fato, dos outros princípios do SOLID é fornecer estrutura, consistência e tornar o código mais sustentável - o que geralmente resulta em algo mais simples.

Ben Cottrell
fonte
Acho que essa resposta abordou o cerne do meu problema, obrigado. Eu tenho usado para refatorar partes do meu aplicativo e as coisas estão muito mais limpas até agora.
precisa saber é o seguinte
1
Você precisa ter cuidado com os modelos anêmicos , é perfeitamente aceitável que o modelo de personagem tenha um comportamento como Walk, Attacke Duck. O que não está bem, é ter Savee Load(persistência). O SRP declara que uma classe deve ter apenas uma responsabilidade, mas a responsabilidade do Personagem é ser um personagem, não um contêiner de dados.
Chris Wohlert
1
@ChrisWohlert Esse é o motivo do nome CharacterModel, cuja responsabilidade é ser um contêiner de dados para dissociar as preocupações da camada de dados da camada de lógica de negócios. De fato, ainda pode ser desejável que uma Characterclasse comportamental exista em algum lugar também, mas com 80 ações e 30 comandos, eu me inclinaria a quebrá-la ainda mais. Na maioria das vezes, acho que os substantivos de entidade são um "arenque vermelho" para os nomes das classes porque é difícil extrapolar a responsabilidade de um substantivo de entidade, e é muito fácil para eles se transformarem em um tipo de canivete suíço.
Ben Cottrell
10

Você sempre pode usar uma definição mais abstrata de "responsabilidade". Essa não é uma maneira muito boa de julgar essas situações, pelo menos até que você tenha muita experiência nisso. Observe que você fez facilmente quatro pontos, o que eu chamaria de melhor ponto de partida para a granularidade da sua classe. Se você está realmente seguindo o SRP, é difícil fazer pontos como esse.

Outra maneira é olhar para os alunos e se separar com base nos métodos que realmente os utilizam. Por exemplo, crie uma classe com todos os métodos que realmente usam self.timer, outra classe com todos os métodos que realmente usam self.outgoinge outra classe com o restante. Faça outra classe com seus métodos que usem uma referência db como argumento. Quando suas aulas são muito grandes, geralmente existem grupos como este.

Não tenha medo de dividi-lo em tamanho menor do que você acha razoável como experimento. É para isso que serve o controle de versão. O ponto de equilíbrio certo é muito mais fácil de ver depois de levar muito longe.

Karl Bielefeldt
fonte
3

A definição de "responsabilidade" é notoriamente vaga, mas se torna um pouco menos vaga se você pensar nela como uma "razão para mudar". Ainda vago, mas você pode analisar um pouco mais diretamente. Os motivos da mudança dependem do seu domínio e de como o software será usado, mas os jogos são bons exemplos de casos, porque você pode fazer suposições razoáveis ​​sobre isso. No seu código, conto cinco responsabilidades diferentes nas cinco primeiras linhas:

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

Sua implementação será alterada se os requisitos do jogo mudarem de uma destas maneiras:

  1. A noção do que constitui um "jogo" muda. Isso pode ser o menos provável.
  2. Como você mede e rastreia alterações nos pontos de integridade
  3. O seu sistema de ataque muda
  4. O seu sistema de pontos muda
  5. O seu sistema de temporização muda

Você está carregando de bancos de dados, resolvendo ataques, vinculando-se a jogos, cronometrando coisas; parece-me que a lista de responsabilidades já é muito longa e só vimos uma pequena parte da sua Characterturma. Portanto, a resposta para uma parte da sua pergunta é não: sua classe quase certamente não segue o SRP.

No entanto, eu diria que há casos em que é aceitável no SRP ter uma classe de 2.500 linhas ou mais. Alguns exemplos podem ser:

  • Um cálculo matemático altamente complexo, mas bem definido, que recebe entrada bem definida e retorna saída bem definida. Este poderia ser um código altamente otimizado que precisa de milhares de linhas. Métodos matemáticos comprovados para cálculos bem definidos não têm muitos motivos para mudar.
  • Uma classe que atua como um armazenamento de dados, como uma classe que apenas possui yield return <N>os primeiros 10.000 números primos ou as 10.000 palavras mais comuns em inglês. Existem possíveis razões pelas quais essa implementação seria preferível a extrair de um armazenamento de dados ou arquivo de texto. Essas classes têm muito poucas razões para mudar (por exemplo, você acha que precisa de mais de 10.000).
Carl Leth
fonte
2

Sempre que você trabalha com alguma outra entidade, pode introduzir um terceiro objeto que faz o tratamento.

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)

Aqui você pode introduzir um 'AttackResolver' ou algo parecido com essas linhas que lida com o envio e a coleta de estatísticas. O on_attack aqui é apenas sobre o estado do personagem que está fazendo mais?

Você também pode revisitar o estado e se perguntar se parte do estado que você realmente precisa estar no personagem. 'successful_attack' soa como algo que você poderia rastrear em outra classe também.

Joppe
fonte