Justificativa para preferir variáveis ​​locais sobre variáveis ​​de instância?

A base de código em que estou trabalhando frequentemente usa variáveis ​​de instância para compartilhar dados entre vários métodos triviais. O desenvolvedor original está convencido de que isso segue as melhores práticas declaradas no livro Código Limpo do tio Bob / Robert Martin: "A primeira regra das funções é que elas devem ser pequenas". e "O número ideal de argumentos para uma função é zero (niládico). (...) Os argumentos são difíceis. Eles exigem muito poder conceitual".

Um exemplo:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  private byte[] encodedData;
  private EncryptionInfo encryptionInfo;
  private EncryptedObject payloadOfResponse;
  private URI destinationURI;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    getEncodedData(encryptedRequest);
    getEncryptionInfo();
    getDestinationURI();
    passRequestToServiceClient();

    return cryptoService.encryptResponse(payloadOfResponse);
  }

  private void getEncodedData(EncryptedRequest encryptedRequest) {
    encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private void getEncryptionInfo() {
    encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
  }

  private void getDestinationURI() {
    destinationURI = router.getDestination().getUri();
  }

  private void passRequestToServiceClient() {
    payloadOfResponse = serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }
}

Eu refatoraria isso da seguinte maneira usando variáveis ​​locais:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    byte[] encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
    EncryptionInfo encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
    URI destinationURI = router.getDestination().getUri();
    EncryptedObject payloadOfResponse = serviceClient.handle(destinationURI, encodedData,
      encryptionInfo);

    return cryptoService.encryptResponse(payloadOfResponse);
  }
}

Isso é mais curto, elimina o acoplamento implícito de dados entre os vários métodos triviais e limita os escopos das variáveis ​​ao mínimo necessário. Apesar desses benefícios, ainda não consigo convencer o desenvolvedor original de que essa refatoração é justificada, pois parece contradizer as práticas do tio Bob mencionadas acima.

Daí minhas perguntas: qual é o objetivo, a justificativa científica de favorecer variáveis ​​locais sobre variáveis ​​de instância? Eu não consigo colocar o dedo nele. Minha intuição me diz que os acoplamentos ocultos são ruins e que um escopo estreito é melhor que um amplo. Mas qual é a ciência para apoiar isso?

E, inversamente, há alguma desvantagem nessa refatoração que eu possivelmente tenha esquecido?

Qual é o objetivo, a lógica científica de favorecer variáveis ​​locais sobre variáveis ​​de instância?

O escopo não é um estado binário, é um gradiente. Você pode classificá-las do maior para o menor:

Global > Class > Local (method) > Local (code block, e.g. if, for, ...)

Edit: o que eu chamo de "escopo de classe" é o que você quer dizer com "variável de instância". Que eu saiba, esses são sinônimos, mas eu sou um desenvolvedor de C #, não um desenvolvedor de Java. Por uma questão de brevidade, agrupei todas as estatísticas na categoria global, pois as estatísticas não são o tópico da questão.

Quanto menor o escopo, melhor. A lógica é que as variáveis ​​devem viver no menor escopo possível . Existem muitos benefícios para isso:

• Obriga você a pensar na responsabilidade da classe atual e ajuda a manter o SRP.
• Ele permite que você não tem que evitar conflitos de nomes globais, por exemplo, se duas ou mais classes têm uma Namepropriedade, você não é obrigado a prefixar-los como FooName, BarName... Assim, mantendo os seus nomes de variáveis como limpa e concisa possível.
• Ele organiza o código limitando as variáveis ​​disponíveis (por exemplo, para o Intellisense) àquelas que são contextualmente relevantes.
• Ele permite alguma forma de controle de acesso para que seus dados não possam ser manipulados por algum ator que você não conhece (por exemplo, uma classe diferente desenvolvida por um colega).
• Isso torna o código mais legível, pois você garante que a declaração dessas variáveis ​​tente permanecer o mais próximo possível do uso real dessas variáveis.
• Declarar arbitrariamente variáveis ​​em um escopo excessivamente amplo geralmente é indicativo de um desenvolvedor que não entende bem o POO ou como implementá-lo. Ver variáveis ​​com escopo muito amplo atua como sinalizador de que provavelmente há algo de errado com a abordagem OOP (com o desenvolvedor em geral ou com a base de código em específico).
• (Comentário por Kevin) O uso de locais obriga você a fazer as coisas na ordem certa. No código original (variável de classe), você poderia mover passRequestToServiceClient()- se incorretamente para a parte superior do método, e ele ainda seria compilado. Com os habitantes locais, você só poderia cometer esse erro se passasse uma variável não inicializada, o que é esperançosamente óbvio o suficiente para que você não faça isso de verdade.

Apesar desses benefícios, ainda não consigo convencer o desenvolvedor original de que essa refatoração é justificada, pois parece contradizer as práticas do tio Bob mencionadas acima.

E, inversamente, há alguma desvantagem nessa refatoração que eu possivelmente tenha esquecido?

O problema aqui é que seu argumento para variáveis ​​locais é válido, mas você também fez alterações adicionais que não estão corretas e fazem com que a correção sugerida falhe no teste de odor.

Embora eu entenda a sua sugestão "sem variável de classe" e tenha mérito, na verdade, você também removeu os métodos, e esse é um jogo totalmente diferente. Os métodos deveriam ter permanecido e, em vez disso, você deveria alterá-los para retornar seu valor em vez de armazená-lo em uma variável de classe:

private byte[] getEncodedData() {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
}

private EncryptionInfo getEncryptionInfo() {
    return cryptoService.getEncryptionInfoForDefaultClient();
}

// and so on...

Concordo com o que você fez no processmétodo, mas você deveria estar chamando os submódulos particulares em vez de executar seus corpos diretamente.

public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    byte[] encodedData = getEncodedData();
    EncryptionInfo encryptionInfo = getEncryptionInfo();

    //and so on...

    return cryptoService.encryptResponse(payloadOfResponse);
}

Você deseja essa camada extra de abstração, especialmente quando se deparar com métodos que precisam ser reutilizados várias vezes. Mesmo se você atualmente não reutilizar seus métodos , ainda é uma boa prática criar já os submódulos quando relevantes, mesmo que apenas para ajudar na legibilidade do código.

Independentemente do argumento da variável local, notei imediatamente que sua correção sugerida é consideravelmente menos legível que a original. Eu admito que o uso indevido de variáveis ​​de classe também diminui a legibilidade do código, mas não à primeira vista em comparação com você ter empilhado toda a lógica em um único método (agora muito complicado).

O código original está usando variáveis ​​de membro como argumentos. Quando ele diz para minimizar o número de argumentos, o que ele realmente quer dizer é minimizar a quantidade de dados que os métodos requerem para funcionar. Colocar esses dados em variáveis ​​membro não melhora nada.

Outras respostas já explicaram perfeitamente os benefícios das variáveis ​​locais; portanto, tudo o que resta é essa parte da sua pergunta:

Apesar desses benefícios, ainda não consigo convencer o desenvolvedor original de que essa refatoração é justificada, pois parece contradizer as práticas do tio Bob mencionadas acima.

Isso deve ser fácil. Basta apontar para a seguinte citação no Código Limpo do tio Bob:

Não tem efeitos colaterais

Efeitos colaterais são mentiras. Sua função promete fazer uma coisa, mas também faz outras coisas ocultas. Às vezes, ele faz alterações inesperadas nas variáveis ​​de sua própria classe. Às vezes, os parâmetros são passados ​​para a função ou para os globais do sistema. Em ambos os casos, são enganosas e desonestas, que frequentemente resultam em estranhos acoplamentos temporais e dependências de ordem.

(exemplo omitido)

Esse efeito colateral cria um acoplamento temporal. Ou seja, checkPassword só pode ser chamado em determinados horários (em outras palavras, quando é seguro inicializar a sessão). Se for chamado fora de ordem, os dados da sessão podem ser perdidos inadvertidamente. Os acoplamentos temporais são confusos, especialmente quando ocultos como efeito colateral. Se você precisar de um acoplamento temporal, deixe claro no nome da função. Nesse caso, podemos renomear a função checkPasswordAndInitializeSession, embora isso certamente viole "Faça uma coisa".

Ou seja, o tio Bob não diz apenas que uma função deve ter poucos argumentos, ele também diz que as funções devem evitar interagir com o estado não local sempre que possível.

"Isso contradiz o que o tio de alguém pensa" NUNCA é um bom argumento. NUNCA. Não tome sabedoria dos tios, pense por si mesmo.

Dito isto, variáveis ​​de instância devem ser usadas para armazenar informações que realmente precisam ser armazenadas permanentemente ou semi-permanentemente. A informação aqui não é. É muito simples viver sem as variáveis ​​de instância, para que elas possam ir.

Teste: escreva um comentário da documentação para cada variável de instância. Você pode escrever algo que não seja completamente inútil? E escreva um comentário da documentação para os quatro acessadores. Eles são igualmente inúteis.

O pior é que assuma a maneira de descriptografar as alterações, porque você usa um cryptoService diferente. Em vez de precisar alterar quatro linhas de código, você deve substituir quatro variáveis ​​de instância por diferentes, quatro getters por diferentes e alterar quatro linhas de código.

Mas é claro que a primeira versão é preferível se você for pago pela linha de código. 31 linhas em vez de 11 linhas. Três vezes mais linhas para escrever e manter para sempre, para ler quando você estiver depurando algo, para se adaptar quando forem necessárias alterações, para duplicar se você suportar um segundo cryptoService.

(Perdeu o ponto importante de que o uso de variáveis ​​locais obriga a fazer as chamadas na ordem certa).

Qual é o objetivo, a lógica científica de favorecer variáveis ​​locais sobre variáveis ​​de instância? Eu não consigo colocar o dedo nele. Minha intuição me diz que os acoplamentos ocultos são ruins e que um escopo estreito é melhor que um amplo. Mas qual é a ciência para apoiar isso?

As variáveis ​​de instância são para representar as propriedades de seu objeto host, não para representar propriedades específicas para encadeamentos de computação com escopo mais restrito que o próprio objeto. Algumas das razões para se fazer essa distinção que parece não ter sido abordada giram em torno da simultaneidade e da reentrada. Se os métodos trocam dados configurando os valores das variáveis ​​de instância, dois threads simultâneos podem facilmente se desvalorizar para os valores dessas variáveis ​​de instância, gerando erros intermitentes e difíceis de encontrar.

Somente um encadeamento sozinho pode encontrar problemas nesse sentido, porque há um alto risco de um padrão de troca de dados baseado em variáveis ​​de instância tornar os métodos não reentrantes. Da mesma forma, se as mesmas variáveis ​​forem usadas para transmitir dados entre diferentes pares de métodos, existe o risco de que um único encadeamento que execute mesmo uma cadeia não-recursiva de invocações de métodos encontre erros que giram em torno de modificações inesperadas das variáveis ​​de instância envolvidas.

Para obter resultados corretos de maneira confiável em um cenário como esse, você precisa usar variáveis ​​separadas para se comunicar entre cada par de métodos em que um invoca o outro ou para que cada implementação de método leve em consideração todos os detalhes de implementação de todos os outros. métodos que invoca, direta ou indiretamente. Isso é quebradiço e tem uma escala ruim.

Discutindo apenas process(...), o exemplo de seus colegas é muito mais legível no sentido da lógica de negócios. Por outro lado, o seu exemplo contrário leva mais do que uma rápida olhada para extrair qualquer significado.

Dito isto, o código limpo é legível e de boa qualidade - empurrar o estado local para um espaço mais global é apenas uma montagem de alto nível, portanto, é zero a qualidade.

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest request) {
    checkNotNull(encryptedRequest);

    return encryptResponse
      (routeTo
         ( destination()
         , requestData(request)
         , destinationEncryption()
         )
      );
  }

  private byte[] requestData(EncryptedRequest encryptedRequest) {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private EncryptionInfo destinationEncryption() {
    return cryptoService.getEncryptionInfoForDefaultClient();
  }

  private URI destination() {
    return router.getDestination().getUri();
  }

  private EncryptedObject routeTo(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {
    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }

  private void encryptResponse(EncryptedObject payloadOfResponse) {
    return cryptoService.encryptResponse(payloadOfResponse);
  }
}

Esta é uma versão que remove a necessidade de variáveis ​​em qualquer escopo. Sim, o compilador irá gerá-los, mas a parte importante é que ele controla isso para que o código seja eficiente. Embora também seja relativamente legível.

Apenas um ponto na nomeação. Você deseja o nome mais curto que seja significativo e expanda as informações já disponíveis. ie destinationURI, o 'URI' já é conhecido pela assinatura de tipo.

Gostaria apenas de remover essas variáveis ​​e métodos privados por completo. Aqui está o meu refatorador:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    return cryptoService.encryptResponse(
        serviceClient.handle(router.getDestination().getUri(),
        cryptoService.decryptRequest(encryptedRequest, byte[].class),
        cryptoService.getEncryptionInfoForDefaultClient()));
  }
}

Para o método privado, por exemplo, router.getDestination().getUri()é mais claro e legível que getDestinationURI(). Eu apenas repetiria isso se eu usasse a mesma linha duas vezes na mesma classe. Para ver de outra maneira, se houver necessidade de um getDestinationURI(), provavelmente ele pertence a alguma outra classe, não a SomeBusinessProcessclasse.

Para variáveis ​​e propriedades, a necessidade comum delas é manter valores para serem usados ​​posteriormente. Se a classe não tiver interface pública para as propriedades, elas provavelmente não devem ser propriedades. O pior tipo de uso de propriedades de classe é provavelmente para passar valores entre métodos privados por meio de efeitos colaterais.

De qualquer forma, a classe só precisa fazer process()e, em seguida, o objeto será jogado fora, não há necessidade de manter nenhum estado na memória. Um potencial refatorador adicional seria remover o CryptoService dessa classe.

Com base nos comentários, quero adicionar esta resposta com base na prática do mundo real. De fato, na revisão de código, a primeira coisa que eu escolho é refatorar a classe e mover o trabalho de criptografia / descriptografia. Feito isso, eu perguntaria se os métodos e variáveis ​​são necessários, eles são nomeados corretamente e assim por diante. O código final provavelmente estará mais próximo disso:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;

  public Response process(Request request) {
    return serviceClient.handle(router.getDestination().getUri());
  }
}

Com o código acima, acho que não precisa de refatoração adicional. Assim como as regras, acho que é preciso ter experiência para saber quando e quando não aplicá-las. Regras não são teorias que comprovadamente funcionam em todas as situações.

A revisão de código, por outro lado, tem um impacto real em quanto tempo um código pode passar. Meu truque é ter menos código e facilitar a compreensão. Um nome de variável pode ser um ponto de discussão, se eu puder removê-lo, os revisores nem precisariam pensar nisso.

A resposta de Flater cobre questões de escopo muito bem, mas acho que há outra questão aqui também.

Observe que há uma diferença entre uma função que processa dados e uma função que simplesmente acessa dados .

O primeiro executa a lógica de negócios real, enquanto o último economiza digitação e talvez acrescenta segurança ao adicionar uma interface mais simples e reutilizável.

Nesse caso, parece que as funções de acesso a dados não salvam a digitação e não são reutilizadas em nenhum lugar (ou haveria outros problemas ao removê-las). Portanto, essas funções simplesmente não deveriam existir.

Ao manter somente a lógica de negócios em funções nomeadas, temos o melhor dos dois mundos (em algum lugar entre a resposta de Flater e resposta de imel96 ):

public EncryptedResponse process(EncryptedRequest encryptedRequest) {

    byte[] requestData = decryptRequest(encryptedRequest);
    EncryptedObject responseData = handleRequest(router.getDestination().getUri(), requestData, cryptoService.getEncryptionInfoForDefaultClient());
    EncryptedResponse response = encryptResponse(responseData);

    return response;
}

// define: decryptRequest(), handleRequest(), encryptResponse()

A primeira e mais importante coisa: o tio Bob às vezes parece um pregador, mas afirma que há exceções em suas regras.

Toda a idéia do Código Limpo é melhorar a legibilidade e evitar erros. Existem várias regras que estão se violando.

Seu argumento sobre funções é que funções niládicas são melhores, no entanto, até três parâmetros são aceitáveis. Pessoalmente, acho que 4 também estão ok.

Quando variáveis ​​de instância são usadas, elas devem criar uma classe coerente. Isso significa que as variáveis ​​devem ser usadas em muitos, se não em todos os métodos não estáticos.

Variáveis ​​que não são usadas em muitos lugares da classe devem ser movidas.

Eu não consideraria a versão original nem a refatorada ideal, e o @Flater já declarou muito bem o que pode ser feito com valores de retorno. Melhora a legibilidade e reduz erros para usar valores de retorno.

As variáveis ​​locais reduzem o escopo e, portanto, limitam as maneiras pelas quais as variáveis ​​podem ser usadas e, portanto, ajudam a evitar certas classes de erro e melhoram a legibilidade.

A variável de instância reduz as maneiras pelas quais a função pode ser chamada, o que também ajuda a reduzir determinadas classes de erro e melhora a legibilidade.

Dizer que um está certo e o outro está errado pode muito bem ser uma conclusão válida em qualquer caso particular, mas como conselhos gerais ...

TL; DR: Eu acho que a razão de você cheirar muito zelo é, há muito zelo.

Apesar do fato de que os métodos que começam com get ... não devem retornar nulos, a separação dos níveis de abstrações nos métodos é dada na primeira solução. Embora a segunda solução tenha mais escopo, ainda é mais difícil argumentar sobre o que está acontecendo no método. As atribuições de variáveis ​​locais não são necessárias aqui. Eu manteria os nomes dos métodos e refatoraria o código para algo assim:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    return getEncryptedResponse(
            passRequestToServiceClient(getDestinationURI(), getEncodedData(encryptedRequest) getEncryptionInfo())
        );
  }

  private EncryptedResponse getEncryptedResponse(EncryptedObject encryptedObject) {
    return cryptoService.encryptResponse(encryptedObject);
  }

  private byte[] getEncodedData(EncryptedRequest encryptedRequest) {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private EncryptionInfo getEncryptionInfo() {
    return cryptoService.getEncryptionInfoForDefaultClient();
  }

  private URI getDestinationURI() {
    return router.getDestination().getUri();
  }

  private EncryptedObject passRequestToServiceClient(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {
    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }
}

As duas coisas fazem o mesmo e as diferenças de desempenho são imperceptíveis, então não acho que exista um argumento científico . Tudo se resume a preferências subjetivas então.

E eu também gosto mais do seu jeito do que do seu colega. Por quê? Porque acho que é mais fácil ler e entender, apesar do que dizem os autores de um livro.

Os dois modos realizam a mesma coisa, mas o caminho dele está mais espalhado. Para ler esse código, você precisa alternar entre várias funções e variáveis ​​de membro. Nem tudo está condensado em um só lugar, você precisa se lembrar de tudo isso em sua mente para entender. É uma carga cognitiva muito maior.

Por outro lado, sua abordagem compacta tudo muito mais densamente, mas não para torná-la impenetrável. Você acabou de ler linha após linha, e não precisa memorizar muito para entender.

No entanto, se ele está acostumado a codificar o layout dessa maneira, posso imaginar que, para ele, pode ser o contrário.