Quais são as coisas que tocam instantaneamente os alarmes quando se olha o código? [fechadas]

98

Participei de um evento de artesanato em software há algumas semanas e um dos comentários feitos foi "Tenho certeza de que todos reconhecemos código incorreto quando o vemos" e todos assentiram sabiamente, sem mais discussões.

Esse tipo de coisa sempre me preocupa, pois há aquele truísmo de que todo mundo pensa que é um motorista acima da média. Embora eu ache que reconheço códigos ruins, eu adoraria aprender mais sobre o que as outras pessoas consideram cheiros de código, uma vez que raramente é discutido em detalhes nos blogs das pessoas e apenas em alguns livros. Em particular, acho que seria interessante ouvir algo que cheira a código em um idioma, mas não em outro.

Vou começar com uma pergunta fácil:

Código no controle de origem que possui uma alta proporção de código comentado - por que está lá? foi feito para ser excluído? é um trabalho meio acabado? talvez não devesse ter sido comentado e só foi feito quando alguém estava testando algo? Pessoalmente, acho esse tipo de coisa realmente irritante, mesmo que seja apenas a linha ímpar aqui e ali, mas quando você vê grandes blocos intercalados com o restante do código, é totalmente inaceitável. Também é geralmente uma indicação de que o restante do código provavelmente também terá qualidade duvidosa.

FinnNk
fonte
61
Às vezes, encontro pessoas que comentam código, fazem check-in e dizem "talvez eu precise dele novamente no futuro - se eu removê-lo agora, perderei". Eu tenho que combater com "Er, ... mas é para isso que serve o controle de origem".
talonx
6
Às vezes, principalmente na otimização, é útil deixar o código antigo como um comentário, para que você saiba o que o código otimizado obscuro está substituindo. Pense em deixar um swap de 3 linhas com temp no lugar ao substituí-lo por um swap de um bit de uma linha. (Embora, não vejo necessidade de usar uma linha de swap - SEMPRE, a menos que o tamanho do programa é de importância crítica.)
Chris Cudmore
4
Estou mantendo / limpando o código escrito por um de nossos engenheiros, que codificou algumas coisas úteis, mas admite que não é um programador. Ao consolidar as coisas, comentarei o código antigo e depois analisamos as mudanças e mostro a ele como substituí o dele por algo menor / mais eficiente / mais fácil de entender. Depois, retiro esses blocos e faço o check-in. Ter o código antigo aí traz benefícios, porque ele vê como as coisas podem ser feitas de maneira mais simples e me lembro por que mudei as coisas quando conversamos.
the Tin Man
8
Deixo coisas que "podem ser usadas" em 1 commit, se as coisas não quebrarem ou uma necessidade não for encontrada, ele será removido no próximo commit.
Paul Nathan
24
Hmm. printf("%c", 7)normalmente toca campainhas de alarme para mim. ;)

Respostas:

128
/* Fuck this error */

Normalmente encontrado dentro de um try..catchbloco sem sentido , ele tende a chamar minha atenção. Tão bem quanto /* Not sure what this does, but removing it breaks the build */.

Mais algumas coisas:

  • Várias ifinstruções complexas aninhadas
  • Blocos try-catch usados ​​para determinar um fluxo lógico regularmente
  • Funções com nomes genéricos process, data, change, rework,modify
  • Seis ou sete estilos diferentes de órtese em 100 linhas

Acabei de encontrar:

/* Stupid database */
$conn = null;
while(!$conn) {
    $conn = mysql_connect("localhost", "root", "[pass removed]");
}
/* Finally! */
echo("Connected successfully.");

Certo, porque ter que usar força bruta em suas conexões MySQL é a maneira certa de fazer as coisas. Acontece que o banco de dados estava tendo problemas com o número de conexões, então eles continuaram atingindo o tempo limite. Em vez de depurar isso, eles simplesmente tentaram repetidamente até que funcionasse.

Josh K
fonte
19
Se ao menos eu pudesse votar isso 6 vezes! Todos bons exemplos. Também não gosto de comentários arrogantes / engraçados (especialmente se eles incluem palavrões) - pode ser um pouco divertido na primeira vez que você os lê, mas envelhece (e, portanto, distrai) muito rapidamente.
FinnNk
5
Gosto do seu exemplo, embora eu diria que, em certos contextos, várias declarações aninhadas se são inevitáveis. Com muita lógica de negócios, o código pode ser um pouco confuso, mas se o negócio em si é confuso, para simplificar, simplificar o código seria modelar o processo com menos precisão. Como Einstein disse: "As coisas devem ser o mais simples possível e nem um pouco mais simples".
Morgan Herlocker 25/10/10
2
@Prof Plum - Que exemplo você pode dar? Geralmente, a alternativa para vários if's aninhados é dividi-la em (muitos) métodos. Desenvolvedores juniores tendem a evitar isso como se fosse menos desejável que o if; mas geralmente quando pressionados, eles dizem "se fazem isso em menos linhas". É preciso alguém confiante no OOP para intervir e lembrá-lo de que menos linhas! = Código melhor.
STW
2
@STW Esse é um bom argumento, no entanto, eu diria que depende da profundidade do assentamento. Eu certamente concordaria que qualquer coisa com mais de três ninhos de profundidade muitas vezes precisa de refatoração, porque vai ficar bem peludo. A cotação de seguros, no entanto, é um bom exemplo em que o agrupamento múltiplo pode realmente modelar o mundo real muito bem. Com exceções a determinadas tarifas / prêmios, o manual literalmente lerá algo como "se esta é uma propriedade e se tem uma taxa mínima abaixo de 5,6 e se estiver na Carolina do Norte e se tiver um barco no local, faça isso e tal." com muitas outras opções.
Morgan Herlocker 25/10/10
4
@ Josh, se "eles" são colegas, então eu iria refletir sobre por que você não disse "nós" ...
104

A principal bandeira vermelha para mim são blocos de código duplicados, porque mostra que a pessoa não entende os fundamentos da programação ou estava com muito medo de fazer as alterações apropriadas em uma base de código existente.

Eu também costumava contar a falta de comentários como uma bandeira vermelha, mas recentemente trabalhei em um monte de código muito bom, sem comentários.

Ben Hughes
fonte
1
+1: Eu vi algum código do colega que se considerava um especialista em Linux, que escreveu um aplicativo de loop simples como uma função longa, a coisa toda repetidamente em main (). Caramba.
KFro
4
@KFro, isso é desenrolar o loop. Isso é o que os compiladores fazem o tempo todo :) MUITO eficiente!
3
@Thorbjorn: Às vezes, você precisa ajudar um pouco o compilador; afinal, você é o humano esperto e ele é apenas um computador idiota.
yatima2975
3
Outro motivo que eu vi: o consultor foi pago para implementar o recurso o mais rápido possível (é por isso que também faltam testes e documentação). Copiar / colar é mais rápido do que pensar em como fazer as coisas corretamente.
LennyProgrammers
4
Evitar a duplicação de código pode ser uma obsessão. Em uma linguagem como C ++, nem sempre é tão fácil quanto deve ser fatorar as diferentes partes, mas ainda assim ter um código robusto e eficiente. Às vezes, um pouco de recortar e colar é a opção mais prática. Além disso, o princípio de otimização pode ser aplicado - recortar e colar pode fornecer uma solução rápida e fácil que você pode refatorar mais tarde, se necessário. Você pode estar economizando uma dor de cabeça de manutenção para mais tarde, mas sabe com certeza que está evitando atrasos no momento.
Steve314
74

Código que tenta mostrar o quão inteligente é o programador, apesar de não agregar valor real:

x ^= y ^= x ^= y;
Rei Miyasaka
fonte
12
Uau, isso é tão muito mais legível do queswap(x, y);
JBRWilkinson
8
se xey são indicadores, e essa atribuição ocorre por um período razoável de tempo, ele quebra profundamente os coletores de lixo conservadores como o Boehm GC.
SingleNegationElimination
12
A propósito, esse código não foi definido em C e C ++ devido a várias alterações sem um ponto de sequência intermediário.
Fredoverflow
9
Olhando para isso, a única coisa que me veio à mente foram os emoticons:^_^
Darien
62
  • 20.000 funções de linha (exagero). Qualquer função que precise de mais do que duas telas precisa ser fatorada novamente.

  • Na mesma linha, arquivos de classe que parecem durar para sempre. Provavelmente, existem alguns conceitos que poderiam ser abstraídos em classes que esclareceriam o objetivo e a função da classe original e provavelmente onde ela está sendo usada, a menos que sejam todos métodos internos.

  • variáveis ​​não descritivas, não triviais ou muitas variáveis ​​não descritivas triviais. Isso torna deduzir o que realmente está acontecendo como um enigma.

Dominique McDonnell
fonte
9
Costumo limitar as funções a 1 tela sempre que possível.
Matt DiTrolio
20
1 tela é até um trecho. Começo a me sentir suja depois de 10 ou mais linhas.
Bryan Rowe
54
Ok, vou expressar o que pode ser uma opinião impopular. Eu digo que é um cheiro de código escrever funções que são processos atômicos, de cima para baixo, que são divididos em funções separadas porque o desenvolvedor está se apegando a algumas "funções que devem ser curtas". As funções devem ser quebradas ao longo de linhas FUNCIONAIS, não apenas porque devem ter algum "tamanho certo" mítico. É por isso que eles são chamados de FUNÇÕES.
Dan Ray
7
@ Dan, as funções não devem ser curtas apenas para serem curtas, mas há apenas tanta informação que você pode guardar em sua cabeça ao mesmo tempo. Talvez eu tenha um cérebro pequeno, pois para mim esse limite é um par de telas :). Quebrar funções em várias funções quando elas começam a testar se é necessário um limite para evitar erros. Por um lado, fornece encapsulamento para que você possa pensar em um nível superior e, por outro, oculta o que está acontecendo, tornando mais difícil descobrir como a função funciona. Penso que a quebra de funções deve ser feita para facilitar a legibilidade, para não se ajustar a um "comprimento perfeito".
Dominique McDonnell
6
@ Dominic, @ Péter, acho que nós três estamos realmente dizendo a mesma coisa. Quando há uma boa razão para colocar o código em uma função menor, eu sou a favor. O que estou rejeitando é falta por falta de design de funções. Você sabe, uma pilha de chamadas três vezes mais do que precisa, mas pelo menos essas funções são curtas. Eu prefiro depurar uma função alta que faz uma coisa bem e claramente do que perseguir o caminho de execução através de uma dúzia de funções encadeadas que são chamadas apenas a partir da anterior.
Dan Ray
61
{ Let it Leak, people have good enough computers to cope these days }

O pior é que é de uma biblioteca comercial!

Reallyethical
fonte
32
Isso não soa alarme. Praticamente chuta você entre as pernas.
Steven Evers
15
Filha é viciada em metanfetamina. Quem se importa, pelo menos ela não ficará obesa. :: susan ::
Evan Plaice
17
Quando me encontro em tempos difíceis, a mãe buntu vem até mim. Falando palavras de sabedoria, deixe vazar. DEIXE VAZAR .. DEIXE VAZAR. DEIXE VAZAR oh DEIXE VAZAR. Se apenas vazar uma vez, não é um vazaaaaak. (se você leu até aqui, +1). Eu realmente preciso considerar descafeinado.
Tim Post
13
"À medida que o prazo se aproxima, os vazamentos são tudo o que posso ver, em algum lugar alguém sussurra: 'Escreva em C ...... eeeeee !!!'"
chiurox
9
Era uma vez dois sistemas operacionais. Um vazou e caiu se durou mais de 49 dias, o outro foi perfeito e funcionaria para sempre. Um foi lançado em 1995 para um grande parque de fãs e foi usado por milhões de pessoas - o outro nunca foi lançado porque ainda está verificando se não há bugs. Há uma diferença entre filosofia e engenharia.
Martin Beckett
53

Comentários tão detalhados que, se houvesse um compilador em inglês, ele seria compilado e executado perfeitamente, mas não descreve nada que o código não.

//Copy the value of y to temp.
temp = y;
//Copy the value of x to y.
y = x;
//Copy the value of temp to x.
x = temp;

Além disso, comentários sobre códigos que poderiam ter sido eliminados tiveram o código aderido a algumas diretrizes básicas:

//Set the age of the user to 13.
a = 13;
Rei Miyasaka
fonte
15
Há, é chamado de COBOL :-)
Gaius
26
O segundo caso não é o pior. O pior é: / * Defina a idade do usuário como 13 * / a = 18;
PhiLho
4
@ PhiLho - não, pior ainda é quando o /do */está faltando, então todo o código até o final do próximo */é ignorado. Felizmente, o destaque da sintaxe torna esse tipo de coisa raro atualmente.
Steve314
3
Pior novamente, apara user_age? Realmente?
glasnt
2
Eu costumava manter um documento padrão de código em um empregador anterior, uma seção da qual era apropriado comentar. Meu exemplo favorito foi do MSDN:i = i + 1; //increment i
Michael Itzoe 15/11/2012
42

Código que produz avisos quando compilado.

Rei Miyasaka
fonte
1
Há um candidato para adicionar a opção do compilador 'Todos os avisos como erros' ao makefile / projeto.
JBRWilkinson
3
Eu acho que isso poderia ser útil se você estiver em um projeto com várias pessoas nas quais você simplesmente não confia - embora se eu fosse participar de um projeto em que essa opção é definida, isso por si só me preocuparia com a capacidade do outro programadores são.
Rei Miyasaka
1
Eu discordo disso. Alguns avisos do compilador (como a comparação entre assinado e não assinado, quando você sabe que os dois valores não têm sinal, mesmo que os tipos sejam diferentes) são preferíveis aos códigos de lixo com vazamentos desnecessários. Se eu reduzir o código usando um número inteiro com sinal portátil que uma função modifique apenas se o número inteiro tiver um valor não assinado, eu farei isso.
Tim Post
13
Prefiro desordenar meu código com quase supérfluo do (unsigned int)que desordenar minhas listas de aviso / erro com avisos benignos. Eu odiaria que a lista de avisos se tornasse um ponto cego. É também muito mais de um PITA explicar para outras pessoas por que você está ignorando um aviso do que está a explicar por que você está fazendo um elenco de gás natural intspara unsigned ints.
Rei Miyasaka
Ocasionalmente, você precisa trabalhar com uma API que vomita erros, independentemente do que você faz. Exemplos clássicos são onde a API é definida em termos de itens quebrados pelo design (algumas constantes ioctl () antigas eram assim e, às vezes, os desenvolvedores de sistemas operacionais insistem em usar o tipo errado em seus cabeçalhos) ou onde depreciam algo sem deixando um bom substituto (obrigado, Apple).
Donal Fellows
36

Funções com números no nome em vez de nomes descritivos , como:

void doSomething()
{
}

void doSomething2()
{
}

Por favor, faça com que os nomes das funções signifiquem algo! Se doSomething e doSomething2 fizerem coisas semelhantes, use nomes de funções que diferenciam as diferenças. Se doSomething2 for uma quebra de funcionalidade do doSomething, nomeie-o por sua funcionalidade.

Wonko the Sane
fonte
Da mesma forma, @Parm for SQL
Dave
2
+1 - Enter mshtml- quebra meus olhos :(
Kyle Rozendo
3
A exceção a isso seria o código da GUI. Por exemplo, se um formulário de correio tradicional tivesse dois endereços; endereço1 e endereço2 é mais razoável que endereço e endereço alternativo. Etiquetas falsas que são apenas estáticas também são uma exceção razoável também.
Evan Plaice
@Evan - bastante justo, embora eu estivesse fazendo mais uma distinção na funcionalidade.
Wonko the Sane
1
+1 - Eu já vi isso usado como método de controle de pseudo-versão.
EZ Hart
36

Números mágicos ou cordas mágicas.

   if (accountBalance>200) { sendInvoice(...); }

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }
JohnFx
fonte
4
Não é tão ruim assim com os dois segundos, pelo menos o desconto é explicado e o "Vencido" é intuitivo. A 200, por outro lado ...
Tarka
9
@ Slokun - Intuitivo não é o ponto, mas manutenção e fragilidade. Por exemplo, considere o que acontece quando o valor do desconto é alterado e 0,9 é codificado em seis lugares diferentes. Além disso, o uso de strings em vez de constantes / enumerações está causando problemas em idiomas com strings com distinção entre maiúsculas e minúsculas.
JohnFx
+1 Passei muito tempo depurando um problema que acabou sendo causado por uma linha 'timeout = 15;' enterrado em um programa.
Aubreyrhodes
Acho que o último às vezes é bom, dependendo de onde vieram os dados do invoiceStatus. Se vier de uma API pública que retorna o JSON que foi decodificado, provavelmente a verificação em uma constante String codificada está bem. Concordo que "200" e "0,9" são apenas constantes mágicas e não devem ser codificados dessa maneira. Mesmo se eles forem usados ​​apenas neste local, a manutenção será mais fácil se você os definir em uma seção de configuração separadamente, em vez de intercalá-los no código lógico. E se eles são usados ​​em vários locais, a manutenção se torna muito mais fácil.
Ben Lee
36
  • Talvez não seja o pior, mas mostra claramente o nível dos implementadores:

    if(something == true) 
  • Se uma linguagem possui uma construção de loop for ou iterador, o uso de um loop while também demonstra o nível de entendimento dos implementadores da linguagem:

    count = 0; 
    while(count < items.size()){
       do stuff
       count ++; 
    }
    
    for(i = 0; i < items.size(); i++){
      do stuff 
    }
    //Sure this is not terrible but use the language the way it was meant to be used.
    
  • A ortografia / gramática inadequadas na documentação / comentários são tão comuns quanto o próprio código. A razão para isso é porque o código foi criado para humanos lerem e máquinas funcionarem. É por isso que usamos linguagens de alto nível, se sua documentação é difícil de obter, faz-me formar preventivamente uma opinião negativa da base de código sem olhar para ela.

Chris
fonte
29

O que noto imediatamente é a frequência de blocos de código profundamente aninhados (if's, while's, etc). Se o código costuma ter mais de dois ou três níveis de profundidade, isso é sinal de um problema de design / lógica. E se tiver 8 ninhos de profundidade, é melhor que haja uma boa razão para não ser quebrado.

GrandmasterB
fonte
6
Eu sei que algumas pessoas aprenderam que todo método deve ter apenas uma returndeclaração, mas quando causa mais de 6 níveis de aninhamento if / then, acho que está fazendo muito mais mal do que bem.
Darien
28

Ao classificar o programa de um aluno, às vezes posso dizer em um momento de "piscar". Estas são as dicas instantâneas:

  • Formatação ruim ou inconsistente
  • Mais de duas linhas em branco em uma linha
  • Convenções de nomenclatura não padronizadas ou inconsistentes
  • Código repetido, quanto mais textuais as repetições, mais forte o aviso
  • O que deveria ser um simples pedaço de código é muito complicado (por exemplo, verificar os argumentos passados ​​para main de maneira complicada)

Raramente minhas primeiras impressões estão incorretas e esses sinais de aviso estão certos cerca de 95% das vezes . Por uma exceção, um aluno novo na linguagem estava usando um estilo de uma linguagem de programação diferente. Escavar e reler o estilo deles no idioma da outra língua removeu os alarmes para mim, e o aluno então recebeu todo o crédito. Mas essas exceções são raras.

Ao considerar um código mais avançado, estes são meus outros avisos:

  • A presença de muitas classes Java que são apenas "estruturas" para armazenar dados. Não importa se os campos são públicos ou privados e usam getters / setters, ainda não é provavelmente parte de um design bem pensado.
  • Classes que têm nomes ruins, como apenas um espaço para nome e não há coesão real no código
  • Referência a padrões de design que nem sequer são usados ​​no código
  • Manipuladores de exceção vazios sem explicação
  • Quando eu pego o código no Eclipse, centenas de "avisos" amarelos alinham o código, principalmente devido a importações ou variáveis ​​não utilizadas

Em termos de estilo, geralmente não gosto de ver:

  • Comentários Javadoc que apenas ecoam o código

Essas são apenas pistas do código incorreto. Às vezes, o que pode parecer um código incorreto não é realmente, porque você não conhece as intenções do programador. Por exemplo, pode haver uma boa razão para algo parecer excessivamente complexo - pode ter havido outra consideração em jogo.

Macneil
fonte
Não vejo como usar o estilo de um idioma em outro é um erro grave (2, 4, 8 espaços por travessão ...). Se o aluno tem pouco outro estilo a seguir, não há nada de errado em ser autoconsistente. Como aluno da série, você vê um bilhão de programas e está no extremo oposto desse espectro, mas essa não é uma razão para descartar totalmente um estilo diferente (mas consistente).
Nick T
18
Não vejo nada errado com as classes que simplesmente agregam dados - isto é, estruturas. É isso que os DTOs (Data Transfer Objects) são e podem tornar o código muito mais legível do que se pode dizer, por exemplo, passando 20 parâmetros para um método.
Nemi
1
Seu comentário sobre estruturas está no local. Tudo bem quando os dados estão em sua forma mais bruta e não serão modificados de forma alguma. Porém, 95% das vezes você deve ter algumas funções na classe para formatar / operar com os dados. Acabei de me lembrar de parte do meu código que essencialmente usa esse padrão e deve ser aprimorado.
usar o seguinte
2
+1 para estilo inconsistente e quebras de linha extras (vi recuos aleatórios, sem recuo, convenções de nomes aleatórios e variáveis ​​e muito mais - e isso no código de produção!). Se você nem se dá ao trabalho de acertar, o que mais você não pode se dar ao trabalho de fazer?
Dean Harding
1
Eu procuro a linha de declaração de um método que está sendo recuado demais em relação ao corpo. Este é um sinal de que eles copiam e colam do arquivo de outra pessoa.
Barry Brown
25

Favorito pessoal / irritação pessoal: nomes gerados pelo IDE que são confirmados. Se o TextBox1 é uma variável importante e importante em seu sistema, você tem outra coisa que vem na revisão de código.

Wyatt Barnett
fonte
25

Variáveis ​​completamente não utilizadas , especialmente quando a variável tem um nome semelhante aos nomes de variáveis ​​usados.

Cruz
fonte
21

Muitas pessoas mencionaram:

//TODO: [something not implemented]

Embora eu desejasse que essas coisas fossem implementadas, pelo menos eles fizeram uma anotação. O que eu acho que é pior é:

//TODO: [something that is already implemented]

Todo é inútil e confuso se você nunca se preocupa em removê-los!

Morgan Herlocker
fonte
1
Acabei de fazer o exercício de produzir um relatório de todos os TODOs em nosso produto de lançamento, além de um plano para descartá-los. Quase metade se tornou obsoleta.
ASHelly 26/10/10
1
Os comentários -1 TODO são usados ​​no MS Visual Studio para rastrear partes do projeto que ainda precisam de trabalho. IE, o IDE mantém uma lista que rastreia TODOs para que você possa clicar neles facilmente e ser levado diretamente para a linha em que o TODO existe. Prefiro ver os TODOs colocados explicitamente para ver o trabalho que ainda precisa ser feito. Veja dotnetperls.com/todo .
quer
3
@Evan Plaice: Uau, você quer dizer que o VS IDE reconhece quando você implementou algo e remove o //TODOcomentário? Impressionante!
JBRWilkinson
4
@Prof Plum Por que não apenas criar uma política em que a pessoa responsável por um TODO coloque seu nome no comentário. Dessa forma, se há algumas sobras que
Evan Solha
3
Melhor planejar do que // TODO usar o rastreador de erros, é para isso!
SingleNegationElimination
20

Um método que exige que eu role para baixo para ler tudo.

BradB
fonte
14
Hmm .. isso depende do que está sendo implementado. Não seria razoável que isso ocorra ao implementar alguns algoritmos complicados, pois é assim que eles são definidos. Obviamente, se a maioria dos métodos é assim, isso é uma bandeira vermelha.
Billy ONeal
9
Como uma declaração geral, discordo: perder tempo constantemente se refatorando para que tudo se encaixe nesse tipo de regra autoimposta realmente aumenta o custo geral do projeto.
Tipo anônimo
1
Não concordo que essa regra possa aumentar o custo geral de um projeto, mas acho que isso é subjetivo, porque dependeria do (s) desenvolvedor (es). Se você mantiver o princípio geral de "separação de preocupações" durante o desenvolvimento, a re-fatoração seria menos uma tarefa se alguém escolhesse fazê-lo. Algo a considerar é quanto custaria três anos depois, quando os desenvolvedores que não codificaram o projeto original estão tentando corrigir um monte de métodos com mais de 300 linhas de código? Quanto aquilo custa?
BradB
18
Fico mais irritado ao rolar para a direita do que para rolar para baixo. O espaço em branco é "livre".
Wonko the Sane 25/10/10
4
Prefiro rolar do que pular todo o arquivo (ou vários arquivos) para descobrir o que o código está fazendo.
TMN
20

Conjunções nos nomes dos métodos:

public void addEmployeeAndUpdatePayrate(...) 


public int getSalaryOrHourlyPay(int Employee) ....

Esclarecimento: A razão pela qual isso soa um alarme é que indica que o método provavelmente viola o princípio da responsabilidade única .

JohnFx
fonte
Hmmm ... se o nome da função define com precisão o que a função faz, então eu discordo. Se o método faz duas coisas separadas que seriam melhores para separar, então eu posso concordar, dependendo do contexto.
Wonko the Sane
2
Essa é a questão. A conjunção implica que é muito provável que faça mais de uma coisa. Pela pergunta, é apenas algo que aumenta minha consciência de que algo PODE estar errado.
JohnFx
3
Agora, e se você tiver que adicionar um funcionário e atualizar sua folha de pagamento em vários lugares? Se esse método contiver duas chamadas de método ( addEmployee(); updatePayrate();), não acho que isso seja um problema.
Matt Grande
13

Obviamente, vincular o código fonte da GPL a um programa comercial de código fechado.

Isso não apenas cria um problema jurídico imediato, mas, na minha experiência, geralmente indica descuido ou despreocupação, o que também se reflete em outras partes do código.

Bob Murphy
fonte
6
Embora seu argumento seja excelente, seu tom é desnecessário.
JBRWilkinson
@JBRWilkinson: Obrigado, você está correto. Minhas desculpas a todos.
Bob Murphy
Eu acho que seu título precisa ser reescrito. Ambos estática e dinamicamente com links para o código-fonte GPL é uma violação da GPL ...
Gavin Coates
Bons pontos. Reescrevi a postagem inteira. Obrigado a todos.
Bob Murphy
9

Agnóstico de idioma:

  • TODO: not implemented
  • int function(...) { return -1; } (o mesmo que "não implementado")
  • Lançando uma exceção por um motivo não excepcional.
  • Uso indevido ou inconsistente de 0, -1ou nullcomo valores de retorno excepcionais.
  • Afirmações sem um comentário convincente dizendo por que nunca deveria falhar.

Idioma específico (C ++):

  • Macros C ++ em minúsculas.
  • Variáveis ​​estáticas ou globais em C ++.
  • Variáveis ​​não inicializadas ou não utilizadas.
  • Qualquer um array newque aparentemente não seja seguro para RAII.
  • Qualquer uso de matriz ou ponteiro que aparentemente não seja seguro. Isso inclui printf.
  • Qualquer uso da parte não inicializada de uma matriz.

Específico do Microsoft C ++:

  • Qualquer nome de identificador que colide com uma macro já definida por qualquer um dos arquivos de cabeçalho do Microsoft SDK.
  • Em geral, qualquer uso da API do Win32 é uma grande fonte de alarmes. Sempre mantenha o MSDN aberto e procure as definições de argumentos / valores de retorno sempre que houver dúvida. (Editado)

C ++ / OOP específico:

  • Herança de implementação (classe concreta) em que a classe pai possui métodos virtuais e não virtuais, sem uma distinção conceitual clara e óbvia entre o que deveria / não deveria ser virtual.
rwong
fonte
18
// TODO: Comente esta resposta
johnc
8
Eu acho que "linguagem independente" significa "C / C ++ / Java" agora?
Inaimathi 25/10/10
+1 "Lançar uma exceção por um motivo não excepcional" não poderia estar mais de acordo!
billy.bob
2
@Inaimathi - Comentários do TODO, stubs de funções, abuso de exceção, confusão de semântica zero / nula / vazia e verificações de sanidade sem sentido são inerentes a todas as linguagens imperativas / OOP e, até certo ponto, a todas as linguagens de programação em geral.
Rei Miyasaka
Acredito que as macros do pré-processador C em minúsculas estejam bem, mas apenas se avaliarem seus argumentos apenas uma vez e resultarem em apenas uma única instrução.
Joe D
8

Estilo de indentação bizarro.

Existem alguns estilos muito populares, e as pessoas levarão esse debate para o túmulo. Mas às vezes vejo alguém usando um estilo de indentação realmente raro ou até caseiro. Isso é um sinal de que eles provavelmente não estão codificando com ninguém além de si mesmos.

Ken
fonte
2
ou apenas um sinal de que eles são um talento individualista altamente valorizado que não foi envolvido na rede de práticas de codificação homogonizadas que não estão relacionadas de modo algum às "melhores práticas".
Tipo anônimo
1
Espero que você esteja sendo sarcástico. Se alguém está codificando de maneira tão incomum, isso deve acionar alarmes. Eles podem ser tão artísticos quanto eles querem, mas ainda assim ... alarmes.
Ken
2
Existe um estilo um tanto incomum (acho que é chamado de estilo Utrecht) que acho bastante útil, apesar de extremamente incomum fora do Haskell / ML / F #. Role para baixo até "module Shapes": learnyouahaskell.com/making-our-own-types-and-typeclasses . O que é legal nesse estilo é que você não precisa modificar o delimitador na linha anterior para adicionar um novo - o que geralmente esqueço de fazer.
Rei Miyasaka
@ReiMiyasaka Sete anos atrasado, mas ... O estilo Utrecht realmente me irrita. Eu acredito que é um erro na especificação Haskell não impor outra "regra de layout" em listas organizadas verticalmente. Dessa forma, o analisador pode detectar uma nova entrada na lista apenas verificando o recuo, que é como todos organizam suas listas de qualquer maneira.
Ryan Reich
@RyanReich Estranho, sete anos depois, e ainda gosto. Eu concordo, no entanto; apesar de todo o embaraço e falhas sintáticas, o F # também permite que os itens sejam delimitados apenas por uma nova linha e recuo (na maioria dos casos), o que cria um código organizado.
Rei Miyasaka
8

Usando muitos blocos de texto em vez de enumerações ou variáveis ​​definidas globalmente.

Não é bom:

if (itemType == "Student") { ... }

Melhor:

private enum itemTypeEnum {
  Student,
  Teacher
}
if (itemType == itemTypeEnum.Student.ToString()) { ... }

Melhor:

private itemTypeEnum itemType;
...
if (itemType == itemTypeEnum.Student) { ... }
Yaakov Ellis
fonte
Ou melhor: use polimorfismo.
Lstor
8

Parâmetros digitados fracamente ou retornam valores nos métodos.

public DataTable GetEmployees() {...}

public DateTime getHireDate(DataTable EmployeeInfo) {...}

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}
JohnFx
fonte
2
+1: Eu tenho que trabalhar com alguns serviços da Web "REST" que retornam tudo como tabelas pseudo-HTML, mesmo quando você passa coisas que são um erro sintático claro. Não autorizado? Entrada de lixo completo? Servidor com excesso de capacidade? 200 (mais uma mensagem em um formato horrível em uma tabela de uma coluna e uma linha). AAaaaaaaaargh!
Donal Fellows
7
  • Vários operadores ternários unidos, então, em vez de se parecer com um if...elsebloco, ele se torna um if...else if...[...]...elsebloco
  • Nomes longos de variáveis ​​sem sublinhados ou camelCasing. Exemplo de algum código que eu peguei:$lesseeloginaccountservice
  • Centenas de linhas de código em um arquivo, com pouco ou nenhum comentário, e o código sendo muito óbvio
  • ifDeclarações excessivamente complicadas . Exemplo do código: if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL))que eu usei paraif ($lessee_obj == null)
Tarka
fonte
7

Cheiro de código: não segue as práticas recomendadas

Esse tipo de coisa sempre me preocupa, pois há aquele truísmo de que todo mundo pensa que é um motorista acima da média.

Aqui está um flash de notícias para você: 50% da população mundial está abaixo da inteligência média. Ok, algumas pessoas teriam exatamente inteligência média, mas não sejamos exigentes. Além disso, um dos efeitos colaterais da estupidez é que você não pode reconhecer sua própria estupidez! As coisas não parecem tão boas se você combinar essas declarações.

Quais são as coisas que tocam instantaneamente os alarmes quando se olha o código?

Muitas coisas boas foram mencionadas e, em geral, parece que não seguir as práticas recomendadas é um cheiro de código.

As melhores práticas geralmente não são inventadas aleatoriamente e geralmente estão lá por uma razão. Muitas vezes isso pode ser subjetivo, mas na minha experiência eles são justificados. Seguir as práticas recomendadas não deve ser um problema, e se você está se perguntando por que elas são como são, pesquise-as em vez de ignorá-las e / ou reclamar delas - talvez seja justificado, talvez não seja.

Um exemplo de prática recomendada pode ser usar curlies em cada bloco if, mesmo que contenha apenas uma linha:

if (something) {
    // whatever
}

Você pode achar que não é necessário, mas li recentemente que é uma das principais fontes de erros. Sempre usando colchetes também foram discutidos no Stack Overflow , e verificar se as instruções if têm colchetes também é uma regra no PMD , um analisador de código estático para Java.

Lembre-se: "Porque é uma prática recomendada" nunca é uma resposta aceitável para a pergunta "por que você está fazendo isso?" Se você não consegue articular por que algo é uma prática recomendada, não é uma prática recomendada, é uma superstição.

Vetle
fonte
2
Isso pode ser pedante, mas acho que importa qual a média que você escolhe. Pelo que entendi, 50% da população mundial está abaixo da inteligência mediana (por definição); mas outras médias não funcionam da mesma maneira. Por exemplo, considere a população (1, 1, 1, 5) que tem uma média aritmética de 2. #
flamingpenguin 25/10/10
hummm, você citou o post "o que superstições-programadores-têm", em que um usuário fez uma declaração ousada sobre aparelhos sem fonte. Não vejo isso como um bom exemplo de melhores práticas.
precisa
@Evan: sim, você está certo. Eu adicionei um pouco mais sobre isso, espero que isso ajude.
Vetle
4
O outro lado disso são as pessoas que seguem servilmente as "melhores práticas" sem qualquer pensamento crítico sobre o motivo pelo qual algo é uma "melhor prática". É por isso que não gosto muito do termo "melhores práticas", porque para algumas pessoas é uma desculpa parar de pensar e seguir o rebanho. "Porque é uma prática recomendada" nunca é uma resposta aceitável para a pergunta "por que você está fazendo isso?" Se você não consegue articular por que algo é uma prática recomendada, não é uma prática recomendada, é uma superstição.
Dan Dyer
Muito bom comentário, Dan! Eu adicionei as duas últimas linhas à resposta.
Vetle
6

Comentários que dizem "isso ocorre porque o design do subsistema froz é totalmente eliminado".

Isso continua ao longo de um parágrafo inteiro.

Eles explicam que o refator a seguir precisa acontecer.

Mas não fez isso.

Agora, eles podem ter sido informados de que não poderiam mudar por seu chefe na época, devido a problemas de tempo ou competência, mas talvez fosse por causa das pessoas serem mesquinhas.

Se um supervisor pensa que j.random. programador não pode fazer uma refatoração, o supervisor deve fazê-lo.

De qualquer forma, isso acontece, eu sei que o código foi escrito por uma equipe dividida, com possíveis políticas de poder, e eles não refatoraram os designs dos subsistemas.

História real. Isso poderia acontecer com você.

Tim Williscroft
fonte
6

Alguém pode pensar em um exemplo em que o código deveria legitimamente se referir a um arquivo por caminho absoluto?

Rei Miyasaka
fonte
1
Esquemas XML contam?
Nick T
1
Arquivos de configuração do sistema. Normalmente deve ser definido por ./configure, mas mesmo isso precisa de um valor padrão em algum lugar.
eswald
4
/dev/nulle amigos estão bem. Mas até mesmo coisas como /bin/bashsão suspeitas - e se você é um sistema que tem um jeito estranho /usr/bin/bash?
Tom Anderson
1
O código do cliente de serviço da Web gerado pelas ferramentas JAX-WS (JBossWS e Metro, pelo menos) contém um caminho absoluto conectado ao arquivo WSDL (duas vezes!). O que provavelmente é algo totalmente inapropriado /home/tom/dev/randomhacking/thing.wsdl. É criminalmente insano que esse seja o comportamento padrão.
Tom Anderson
4
about /dev/null: Eu tenho o hábito, ao desenvolver no Windows para manter aplicativos e bibliotecas em baixo c:\dev. De alguma forma, uma pasta nullé sempre criada automaticamente dentro dessa pasta. Juro que não tenho ideia de quem faz isso. (Um dos meus erros favoritos / recursos)
Sean Patrick Floyd
6

Captura de exceções gerais:

try {

 ...

} catch {
}

ou

try {

 ...

} catch (Exception ex) {
}

Uso excessivo da região

Normalmente, o uso de muitas regiões indica que suas aulas são muito grandes. É um sinalizador de aviso que indica que eu deveria investigar mais esse pedaço de código.

Erik van Brakel
fonte
A captura de exceções gerais é apenas um problema se você as reproduzir novamente sem fazer nada. Realmente, para a maioria das coisas, uma classe de exceção seria suficiente. Eu costumo usar runtime_error.
precisa saber é o seguinte
+1 no exemplo 'exceção de captura e descarte'. Se você não estiver fazendo nada com exceção, não pegue. No mínimo, registre-o. No mínimo, coloque um comentário explicando por que não há problema em capturar todas as exceções e seguir em frente nesse ponto do código.
EZ Hart
5

Convenções de nomenclatura de classe que demonstram um entendimento fraco da abstração que estão tentando criar. Ou isso não define uma abstração.

Um exemplo extremo vem à mente em uma classe VB que vi uma vez que foi intitulada Datae tinha mais de 30.000 linhas ... no primeiro arquivo. Era uma classe parcial dividida em pelo menos meia dúzia de outros arquivos. A maioria dos métodos era envolvida em procs armazenados com nomes como FindXByYWithZ().

Mesmo com exemplos menos dramáticos, tenho certeza de que todos acabamos de 'despejar' a lógica em uma classe mal concebida, atribuímos um título totalmente genérico e nos arrependemos mais tarde.

Bryan M.
fonte
5

Funções que reimplementam a funcionalidade básica do idioma. Por exemplo, se você vir um método "getStringLength ()" no JavaScript, em vez de uma chamada para a propriedade ".length" de uma string, você saberá que está com problemas.

Formiga
fonte
5
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...

Obviamente, sem qualquer tipo de documentação e os #defines aninhados ocasionais

Sven
fonte
Ontem vi exatamente esse "padrão" ... no código de produção ... e pior ainda ... no código de produção C ++: - /
Oliver Weiler