Nome para este antipadrão? Campos como variáveis ​​locais [fechadas]

68

Em algum código que estou revisando, estou vendo coisas com o equivalente moral do seguinte:

public class Foo
{
    private Bar bar;

    public MethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public MethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

O campo baraqui é logicamente uma variável local, pois seu valor nunca se destina a persistir nas chamadas de método. No entanto, como muitos dos métodos Fooprecisam de um objeto do tipo Bar, o autor do código original acabou de criar um campo do tipo Bar.

  1. Isso é obviamente ruim, certo?
  2. Existe um nome para esse antipadrão?
JSB ձոգչ
fonte
60
Bem, isso é novo. Espero que essa classe não seja usada em um contexto multithread, ou você terá momentos interessantes pela frente!
TMN
8
Isso é realmente incomum? Eu já vi isso muitas vezes na minha carreira.
JSB #
32
@TMN Não é seguro para threads, mas o pior é que nem sequer é reentrante. Se algum código MethodAou MethodBcausa MethodAou MethodBé chamado de alguma forma (e baré usado novamente, no caso de bar.{A,B}()o infrator), você tem problemas semelhantes, mesmo sem qualquer simultaneidade.
73
Temos que ter um nome para cada coisa idiota que alguém poderia fazer? Apenas chame de má ideia.
Caleb
74
Difícil não chamá-lo de anti-padrão particular suspenso.
Psr

Respostas:

86

Isso é obviamente ruim, certo?

Sim.

  • Isso torna os métodos não reentrantes, o que é um problema se eles forem chamados na mesma instância recursivamente ou em um contexto multithread.

  • Isso significa que o estado de uma chamada vaza para outra (se você esquecer de reinicializar).

  • Isso torna o código difícil de entender porque você precisa verificar o que foi dito acima para ter certeza do que o código está realmente fazendo. (Contraste com o uso de variáveis ​​locais.)

  • Torna cada Fooinstância maior do que precisa. (E imagine fazer isso para N variáveis ​​...)

Existe um nome para esse antipadrão?

Na IMO, isso não merece ser chamado de antipadrão. É apenas um mau hábito de codificação / uso indevido de construções Java / código de porcaria. É o tipo de coisa que você pode ver no código de uma graduação quando o aluno está pulando aulas e / ou não tem aptidão para programar. Se você o vir no código de produção, é um sinal de que precisa fazer muito mais revisões de código ...


Para o registro, a maneira correta de escrever isso é:

public class Foo
{
    public methodA()
    {
        Bar bar = new Bar();  // Use a >>local<< variable!!
        bar.a();
    }

    // Or more concisely (in this case) ...
    public methodB()
    {
        new Bar().b();
    }
}

Observe que também corrigi os nomes de métodos e variáveis ​​para estar em conformidade com as regras de estilo aceitas para identificadores Java.

Stephen C
fonte
11
Eu diria "Se você o vir no código de produção, é um sinal de que deve reexaminar seu processo de contratação "
Beta
@Beta - isso também, embora você possa corrigir maus hábitos de codificação como esse com uma vigorosa revisão de códigos.
Stephen C
+1 por um pouco sobre mais revisões de código. Apesar do que as pessoas pensam, melhorar a prática de contratação não impedirá esse tipo de problema - a contratação é uma porcaria, o melhor que você pode fazer é carregar os dados a seu favor.
mattnz
@StephenC "Torna os métodos não reentrantes, o que é um problema se forem chamados na mesma instância recursivamente ou em um contexto multithread." . Eu sei o que é reentrada, mas não vejo um ponto para isso aqui, pois os métodos não estão usando bloqueios? Você pode explicar por favor?
7117 Geek
@ Geek - Você está se concentrando demais no exemplo específico. Estou falando do caso geral em que o método pode ser chamado de vários threads ou recursivamente.
Stephen C
39

Eu chamaria isso de amplo escopo desnecessário para uma variável. Essa configuração também permite condições de corrida se vários threads acessarem o MethodA e o MethodB.

As 00:00
fonte
12
Especificamente, acho que "escopo variável" é o nome do problema aqui. Essa pessoa precisa aprender o que são variáveis ​​locais e como / quando usá-las. O padrão positivo ou regra geral é usar o menor escopo disponível para cada variável e ampliá-lo apenas conforme necessário. Para ser claro, esse erro não é apenas um estilo ruim. Codificar uma condição de corrida como esta é um erro.
precisa saber é o seguinte
30

Este é um caso específico de um padrão geral conhecido como global doorknobbing. Ao construir uma casa, é tentador comprar apenas uma maçaneta e deixá-la por aí. Quando alguém quer usar uma porta ou armário, simplesmente pega a maçaneta da porta global. Se as maçanetas forem caras, pode ser um bom design.

Infelizmente, quando seu amigo se aproxima e a maçaneta da porta é usada simultaneamente em dois lugares diferentes, a realidade se despedaça. Se a maçaneta da porta é tão cara que você pode pagar apenas uma, vale a pena criar um sistema que permita que as pessoas esperem sua vez pela maçaneta.

Nesse caso, a maçaneta da porta é apenas uma referência, por isso é barato. Se a maçaneta da porta era um objeto real (e eles estavam reutilizando o objeto em vez de apenas a referência), pode ser caro o suficiente para ser um prudent global doorknob. No entanto, quando é barato, é conhecido como a cheapskate global doorknob.

Seu exemplo é da cheapskatevariedade.

Ned Twigg
fonte
19

A principal preocupação aqui seria a simultaneidade - se o Foo estiver sendo usado por vários threads, você terá um problema.

Além disso, é simplesmente bobagem - variáveis ​​locais gerenciam seus próprios ciclos de vida perfeitamente. Substituí-los por variáveis ​​de instância que precisam ser anuladas quando não são mais úteis é um convite para o erro.

Matthew Flynn
fonte
15

É um caso específico de "escopo inadequado", com um lado de "reutilização variável".

ikegami
fonte
7

Não é um anti-padrão. Os antipadrões têm alguma propriedade que faz parecer uma boa ideia, o que leva as pessoas a fazerem isso de propósito; eles são planejados como padrões e depois dá terrivelmente errado.

É também o que gera debates sobre se algo é um padrão, um antipadrão ou um padrão comumente mal aplicado que ainda tem usos em alguns lugares.

Isso está errado.

Para adicionar um pouco mais.

Esse código é supersticioso ou, na melhor das hipóteses, uma prática de culto à carga.

Uma superstição é algo feito sem uma justificativa clara. Pode estar relacionado a algo real, mas a conexão não é lógica.

Uma prática de culto à carga é aquela em que você tenta copiar algo que aprendeu de uma fonte mais experiente, mas na verdade está copiando os artefatos de superfície em vez do processo (chamado de um culto na Papua Nova Guiné que faria rádios de controle de aeronaves de bambu na esperança de fazer os aviões japoneses e americanos da Segunda Guerra Mundial voltarem).

Nos dois casos, não há nenhum argumento real a ser feito.

Um antipadrão é uma tentativa de uma melhoria razoável, seja na pequena (aquela ramificação extra para lidar com o caso extra que precisa ser tratado, que leva ao código de espaguete) ou na grande onde você implementa deliberadamente um padrão que seja desacreditado ou debatido (muitos descreveriam os singletons como tais, com alguns excluindo somente leitura - por exemplo, objetos de log ou somente leitura, por exemplo, objetos de definições de configuração - e alguns condenariam até mesmo esses) - ou então onde você está resolvendo o problema problema errado (quando o .NET foi lançado pela primeira vez, a MS recomendou um padrão para lidar com o descarte quando você tinha campos não gerenciados e campos gerenciados descartáveis ​​- ele realmente lida com essa situação muito bem, mas o problema real é que você tem ambos os tipos de campo na mesma classe).

Como tal, um antipadrão é algo que uma pessoa inteligente que conhece bem o idioma, o domínio do problema e as bibliotecas disponíveis deliberadamente fará, que ainda tem (ou se argumenta ter) uma desvantagem que supera a vantagem.

Como nenhum de nós começa conhecendo bem um determinado idioma, domínio do problema e bibliotecas disponíveis, e como todo mundo pode perder alguma coisa ao passar de uma solução razoável para outra (por exemplo, comece a armazenar algo em um campo para um bom uso e tente refatorá-lo, mas não concluir o trabalho, e você terminará com o código como na pergunta) e, como todos nós perdemos as coisas de tempos em tempos no aprendizado, todos criamos algum código supersticioso ou de culto à carga em algum momento . A coisa boa é que eles são realmente mais claros de identificar e corrigir do que os antipadrões. Os verdadeiros anti-padrões não são, sem dúvida, anti-padrões, ou têm alguma qualidade atraente, ou pelo menos têm alguma maneira de atraí-los, mesmo quando identificados como ruins (muitas e poucas camadas são incontroversamente ruins, mas evitam uma) leva ao outro).

Jon Hanna
fonte
11
Mas as pessoas que pensam que esta é uma boa idéia, provavelmente porque eles pensam que é uma forma de reutilização de código.
JSB
Os iniciantes costumam pensar que declarar duas variáveis ​​de alguma forma requer o dobro do espaço e, portanto, prefere reutilizar variáveis. Isso mostra um mal-entendido do modelo de memória (armazenamento livre vs. pilha, possibilidade de reutilização dos locais da pilha) e otimizações que todos os compiladores são capazes de executar. Eu diria que um iniciante pode, portanto, considerar uma reutilização variável uma boa idéia, para que possa ser classificada como um antipadrão.
Philipp
Isso a torna uma superstição, não um antipadrão.
Jon Hanna
3

(1) Eu diria que não é bom.

Ele está realizando a manutenção manual, que a pilha pode fazer automaticamente com variáveis ​​locais.

Não há benefício de desempenho, pois "novo" é chamado de cada invocação de método.

EDIT: O espaço de memória da classe é maior porque o ponteiro da barra está sempre ocupando memória durante a vida útil da classe. Se o ponteiro da barra fosse local, ele usaria apenas memória durante toda a vida útil da chamada do método. A memória do ponteiro é apenas uma gota no oceano, mas ainda é uma gota desnecessária.

A barra é visível para outros métodos da classe. Métodos que não usam bar não devem ser capazes de vê-lo.

Erros baseados em estado. Nesse caso em particular, os erros da base de estado só devem ocorrer no multiencadeamento, pois "new" é chamado a cada vez.

(2) Não sei se existe um nome para ele, pois na verdade existem vários problemas, não um.
- limpeza manual quando o automático está disponível
- estado desnecessário - estado
que vive mais tempo do que a vida inteira -
visibilidade ou escopo muito altos

mike30
fonte
2

Eu chamaria isso de "Xenofóbico Errante". Ele quer ser deixado em paz, mas acaba sem saber exatamente onde ou o que é. Portanto, é uma coisa ruim, como outros já declararam.

Engenheiro Mundial
fonte
2

Indo na contramão aqui, mas embora eu não considere o exemplo dado um bom estilo de codificação, como está em sua forma atual, o padrão existe durante o teste de unidade.

Essa maneira bastante padrão de fazer testes de unidade

public class BarTester
{
    private Bar bar;

    public Setup() { bar = new Bar(); }
    public Teardown() { bar = null; }

    public TestMethodA()
    {
        bar.A();        
    }

    public TestMethodB()
    {
        bar.B();
    }
}

é apenas uma refatoração desse equivalente do código do OP

public class BarTester
{
    private Bar bar;

    public TestMethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public TestMethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

Eu nunca escreveria código, como é dado pelo exemplo do OP, ele grita refatoração, mas considero o padrão não inválido nem antipadrão .

Lieven Keersmaekers
fonte
Nesse caso, o equipamento é instanciado independentemente para cada teste e os problemas relacionados à reutilização ou simultaneidade variável não ocorrem. No caso geral (fora dos testes de unidade), não se sabe se no máximo um método é chamado em cada objeto.
Philipp
11
@ Philipp - Não discuto o problema de reutilização ou simultaneidade, mas a pergunta do OP era sobre o padrão que é comumente usado em testes de unidade. O fato de o equipamento ser instanciado para cada teste é um detalhe de implementação da estrutura de teste. Mesmo no teste de unidade, o padrão só é seguro quando a estrutura de teste é usada como deveria ser usada. O mesmo raciocínio se aplica ao usar esse padrão no código de produção.
Lieven Keersmaekers #
Não é necessário definir barcomo null, o JUnit de qualquer maneira cria uma nova instância de teste para cada método de teste.
Oberlies
2

Esse cheiro de código é chamado de Campo Temporário na Refatoração por Beck / Fowler.

http://sourcemaking.com/refactoring/temporary-field

Editado para adicionar mais explicações por solicitação dos moderadores: você pode ler mais sobre o cheiro do código no URL referenciado acima ou na cópia impressa do livro. Como o OP estava procurando o nome para esse cheiro antipadrão / código específico, pensei que seria útil citar uma referência até então não mencionada de uma fonte estabelecida com mais de 10 anos de idade, embora eu saiba perfeitamente que estou atrasado para o jogo sobre esta questão, por isso é improvável que a resposta seja votada ou aceita.

Se não for muito promocional, também faço referência e explico esse cheiro específico de código no meu próximo curso do Pluralsight, Fundamentos de refatoração, que espero que seja publicado em agosto de 2013.

Como forma de agregar mais valor, vamos falar sobre como corrigir esse problema específico. Existem várias opções. Se o campo for realmente usado apenas por um método, ele poderá ser rebaixado para uma variável local. No entanto, se vários métodos estiverem usando o campo para se comunicar (em vez de parâmetros), esse é o caso clássico descrito em Refatoração e poderá ser corrigido substituindo o campo por parâmetros ou se os métodos que funcionam com esse código estiverem próximos relacionada, Extract Class pode ser usada para extrair os métodos e os campos para sua própria classe, na qual o campo está sempre em um estado válido (talvez definido durante a construção) e representa o estado dessa nova classe. Se você seguir a rota do parâmetro, mas houver muitos valores para repassar, outra opção é introduzir um novo Objeto de parâmetro,

ssmith
fonte
É importante observar que os campos temporários são frequentemente necessários para uma refatoração de classe de extração. Mas alguém que está ciente disso provavelmente não faria o check-in no código nesse estado intermediário.
Oberlies
1

Eu diria que, quando você se dedica a isso, é realmente uma falta de coesão, e eu posso dar o nome de "Falso Coesão". Eu cunharia (ou, se estou obtendo de algum lugar e esquecendo, "emprestado") esse termo porque a classe parece ser coesa, pois seus métodos parecem operar em um campo de membro. No entanto, na realidade eles não, o que significa que a aparente coesão é realmente falsa.

Quanto a ser ruim ou não, eu diria claramente que é, e acho que Stephen C faz um excelente trabalho para explicar o porquê. No entanto, se ele merece ser chamado de "antipadrão" ou não, acho que ele e qualquer outro paradigma de codificação podem ter um rótulo, pois isso geralmente facilita a comunicação.

Erik Dietrich
fonte
1

Além dos problemas já mencionados, o uso dessa abordagem em ambientes sem coleta automática de lixo (por exemplo, C ++) levaria a vazamentos de memória, pois os objetos temporários não são liberados após o uso. (Embora isso possa parecer um pouco exagerado, existem pessoas por aí que mudariam 'nulo' para 'NULL' e ficariam felizes com a compilação do código.)

peterph
fonte
1

Isso me parece uma péssima aplicação horrível do padrão Flyweight. Infelizmente, eu conheci alguns desenvolvedores que precisam usar a mesma "coisa" várias vezes em métodos diferentes e simplesmente colocá-la em um campo privado em uma tentativa equivocada de economizar memória ou melhorar o desempenho ou outra pseudo-otimização estranha. Em sua mente, eles estão tentando resolver um problema semelhante, pois o Flyweight visa solucionar apenas eles que estão fazendo isso em uma situação em que não é realmente um problema que precisa ser resolvido.

Evicatos
fonte
-1

Eu já o encontrei muitas vezes, geralmente ao atualizar o código anteriormente escrito por alguém que escolheu o VBA For Dummies como seu primeiro título e passou a escrever um sistema relativamente grande em qualquer idioma em que se deparasse.

Dizer que é realmente péssima prática de programação é correto, mas pode ter sido apenas o resultado de não termos recursos revisados ​​pela Internet e por pares, como todos os que desfrutamos hoje, que podem ter guiado o desenvolvedor original por um caminho "mais seguro".

No entanto, ele realmente não merece a tag "antipattern" - mas foi bom ver as sugestões de como o OP poderia ser recodificado.

Lee James
fonte
11
Bem-vindo aos Stack Exchange Programmers, obrigado por inserir sua primeira resposta. Parece que você teve uma votação negativa, possivelmente por causa dos motivos listados nas FAQ programmers.stackexchange.com/faq . O FAQ oferece ótimas sugestões para obter respostas eficazes (e votadas). Às vezes, as pessoas têm a gentileza de adicionar uma nota sobre o voto negativo para indicar se está errado de alguma forma, duplicado, opinião sem evidência ou um eu que repita uma resposta anterior. A maioria de nós teve respostas negadas, fechadas ou removidas para votação, portanto, não se preocupe. É apenas parte de começar. Bem vinda.
DeveloperDon