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.
fonte
printf("%c", 7)
normalmente toca campainhas de alarme para mim. ;)Respostas:
Normalmente encontrado dentro de um
try..catch
bloco 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:
if
instruções complexas aninhadasprocess
,data
,change
,rework
,modify
Acabei de encontrar:
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.
fonte
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.
fonte
Código que tenta mostrar o quão inteligente é o programador, apesar de não agregar valor real:
fonte
swap(x, y);
^_^
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.
fonte
O pior é que é de uma biblioteca comercial!
fonte
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.
Além disso, comentários sobre códigos que poderiam ter sido eliminados tiveram o código aderido a algumas diretrizes básicas:
fonte
/
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.a
para user_age? Realmente?i = i + 1; //increment i
Código que produz avisos quando compilado.
fonte
(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 naturalints
paraunsigned ints
.Funções com números no nome em vez de nomes descritivos , como:
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.
fonte
mshtml
- quebra meus olhos :(Números mágicos ou cordas mágicas.
fonte
200
, por outro lado ...Talvez não seja o pior, mas mostra claramente o nível dos implementadores:
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:
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.
fonte
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.
fonte
return
declaração, mas quando causa mais de 6 níveis de aninhamento if / then, acho que está fazendo muito mais mal do que bem.Ao classificar o programa de um aluno, às vezes posso dizer em um momento de "piscar". Estas são as dicas instantâneas:
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:
Em termos de estilo, geralmente não gosto de ver:
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.
fonte
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.
fonte
Variáveis completamente não utilizadas , especialmente quando a variável tem um nome semelhante aos nomes de variáveis usados.
fonte
Muitas pessoas mencionaram:
Embora eu desejasse que essas coisas fossem implementadas, pelo menos eles fizeram uma anotação. O que eu acho que é pior é:
Todo é inútil e confuso se você nunca se preocupa em removê-los!
fonte
//TODO
comentário? Impressionante!// TODO
usar o rastreador de erros, é para isso!Um método que exige que eu role para baixo para ler tudo.
fonte
Conjunções nos nomes dos métodos:
Esclarecimento: A razão pela qual isso soa um alarme é que indica que o método provavelmente viola o princípio da responsabilidade única .
fonte
addEmployee(); updatePayrate();
), não acho que isso seja um problema.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.
fonte
Agnóstico de idioma:
TODO: not implemented
int function(...) { return -1; }
(o mesmo que "não implementado")0
,-1
ounull
como valores de retorno excepcionais.Idioma específico (C ++):
array new
que aparentemente não seja seguro para RAII.printf
.Específico do Microsoft C ++:
C ++ / OOP específico:
fonte
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.
fonte
Usando muitos blocos de texto em vez de enumerações ou variáveis definidas globalmente.
Não é bom:
Melhor:
Melhor:
fonte
Parâmetros digitados fracamente ou retornam valores nos métodos.
fonte
if...else
bloco, ele se torna umif...else if...[...]...else
bloco$lesseeloginaccountservice
if
Declarações excessivamente complicadas . Exemplo do código:if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL))
que eu usei paraif ($lessee_obj == null)
fonte
Cheiro de código: não segue as práticas recomendadas
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.
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:
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.
fonte
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ê.
fonte
Alguém pode pensar em um exemplo em que o código deveria legitimamente se referir a um arquivo por caminho absoluto?
fonte
/dev/null
e amigos estão bem. Mas até mesmo coisas como/bin/bash
são suspeitas - e se você é um sistema que tem um jeito estranho/usr/bin/bash
?/home/tom/dev/randomhacking/thing.wsdl
. É criminalmente insano que esse seja o comportamento padrão./dev/null
: Eu tenho o hábito, ao desenvolver no Windows para manter aplicativos e bibliotecas em baixoc:\dev
. De alguma forma, uma pastanull
é sempre criada automaticamente dentro dessa pasta. Juro que não tenho ideia de quem faz isso. (Um dos meus erros favoritos / recursos)Captura de exceções gerais:
ou
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.
fonte
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
Data
e 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 comoFindXByYWithZ()
.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.
fonte
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.
fonte
Obviamente, sem qualquer tipo de documentação e os
#define
s aninhados ocasionaisfonte