Quais coisas instantaneamente soam os alarmes ao olhar para o código? [fechadas]

98

Eu participei de um evento de software há algumas semanas e um dos comentários feitos foi "Tenho certeza de que todos reconhecem código ruim quando o vemos" e todos concordaram sabiamente sem mais discussões.

Esse tipo de coisa sempre me preocupa, pois há um truísmo que todo mundo acha que é um motorista acima da média. Embora eu possa reconhecer o código ruim, eu adoraria aprender mais sobre o que outras pessoas consideram ser o código, pois ele raramente é discutido em detalhes nos blogs das pessoas e apenas em alguns livros. Em particular, acho que seria interessante ouvir sobre qualquer coisa que seja um cheiro de código em um idioma, mas não em outro.

Vou começar com um fácil:

Code in source control that has a high proportion of commented out code - why is it there? was it meant to be deleted? is it a half finished piece of work? maybe it shouldn't have been commented out and was only done when someone was testing something out? Personally I find this sort of thing really annoying even if it's just the odd line here and there, but when you see large blocks interspersed with the rest of the code it's totally unacceptable. It's also usually an indication that the rest of the code is likely to be of dubious quality as well.

    
por FinnNk 13.12.2010 / 15:48
fonte

60 respostas

128
/* Fuck this error */

Geralmente encontrado dentro de um bloco try..catch sem sentido, ele tende a atrair minha atenção. Quase tão bem quanto /* Not sure what this does, but removing it breaks the build */ .

Mais algumas coisas:

  • Complexo aninhado múltiplo if declarações
  • Blocos de 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 de contraventamento diferentes em 100 linhas

Um que acabei de encontrar:

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

Certo, porque ter que forçar suas conexões MySQL é o jeito certo de fazer as coisas. Acontece que o banco de dados estava tendo problemas com o número de conexões para que eles continuassem com o tempo limite. Em vez de depurar isso, eles simplesmente tentaram de novo e de novo até que funcionasse.

    
por 29.07.2017 / 04:37
fonte
104

A principal bandeira vermelha para mim são os 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 costumava contar também a falta de comentários como uma bandeira vermelha, mas depois de ter trabalhado muito em código muito bom sem comentários, eu diminuí isso.

    
por 25.10.2010 / 00:53
fonte
74

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

x ^= y ^= x ^= y;
    
por 25.10.2010 / 15:46
fonte
62
  • 20.000 funções de linha (exagero). Qualquer função que precise de mais de um par de telas precisa ser refatorada.

  • Na mesma linha, arquivos de classe que parecem durar para sempre. Existem provavelmente alguns conceitos que poderiam ser resumidos em classes que esclareceriam o propósito 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 triviais não-descritivas. Estes fazem deduzir o que realmente está acontecendo um enigma.

por 25.10.2010 / 02:28
fonte
61
{ Let it Leak, people have good enough computers to cope these days }

O que é pior é que é de uma biblioteca comercial!

    
por 25.10.2010 / 10:33
fonte
53

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

//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, os comentários sobre o código que poderiam ter sido eliminados tiveram o código obedecido a algumas diretrizes básicas:

//Set the age of the user to 13.
a = 13;
    
por 29.07.2017 / 04:38
fonte
42

Código que produz avisos quando compilado.

    
por 25.10.2010 / 15:58
fonte
36

Funciona com números no nome em vez de nomes descritivos , como:

void doSomething()
{
}

void doSomething2()
{
}

Por favor, faça os nomes das funções significarem alguma coisa! Se doSomething e doSomething2 fizerem coisas semelhantes, use nomes de função que diferenciem as diferenças. Se doSomething2 é um quebra de funcionalidade do doSomething, nomeie-o por sua funcionalidade.

    
por 29.07.2017 / 04:39
fonte
36

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

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

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }
    
por 29.07.2017 / 04:40
fonte
36
  • Talvez não seja o pior, mas mostra claramente o nível dos implementadores:

    if(something == true) 
    
  • Se uma linguagem tiver uma construção for loop ou iterator, usar um loop while também demonstrará o nível de compreensão da linguagem pelos implementadores:

    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 falta de ortografia / gramática na documentação / comentários come quase tanto quanto o próprio código. A razão para isso é porque o código era para humanos lerem e máquinas para serem executadas. É por isso que usamos linguagens de alto nível, se a sua documentação é difícil de entender, me faz preemptivamente formar uma opinião negativa sobre a base de código sem olhar para ela.

por 29.07.2017 / 04:41
fonte
29

O que eu noto imediatamente é a freqüência de blocos de código profundamente aninhados (if's, while, etc). Se o código for freqüentemente maior que dois ou três níveis, isso é um sinal de um problema de design / lógica. E se for como 8 ninhos de profundidade, é melhor que haja uma boa razão para não ser quebrada.

    
por 25.10.2010 / 06:17
fonte
28

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

  • Formatação incorreta ou inconsistente
  • Mais de duas linhas em branco em uma linha
  • Convenções de nomenclatura não padrão ou inconsistentes
  • Código repetido, quanto mais as repetições, mais strong será o aviso
  • O que deve ser um simples código é excessivamente complicado (por exemplo, verificar os argumentos passados para o main de uma maneira complicada)

Raramente minhas primeiras impressões estão incorretas e esses avisos estão certos em cerca de 95% do tempo . Por uma exceção, um aluno novo no idioma usava um estilo de uma linguagem de programação diferente. Cavar e reler seu estilo no idioma da outra língua tirou o alarme para mim, e o aluno recebeu crédito total. Mas tais exceções são raras.

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

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

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

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

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

    
por 26.10.2010 / 19:28
fonte
25

Favoritos pessoais / pet peeve: IDE gerou nomes que são empacotados. Se TextBox1 é uma variável importante e importante em seu sistema, você tem outra coisa vindo em breve.

    
por 25.10.2010 / 18:16
fonte
25

Variáveis completamente não usadas , especialmente quando a variável tem um nome semelhante aos nomes das variáveis usadas.

    
por 27.10.2010 / 16:19
fonte
21

Muitas pessoas mencionaram:

//TODO: [something not implemented]

Enquanto eu queria que esse material fosse implementado, pelo menos eles fizeram uma anotação. O que eu acho pior é:

//TODO: [something that is already implemented]

Todo é inútil e confuso se você nunca se incomoda em removê-lo!

    
por 29.07.2017 / 04:43
fonte
20

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

    
por 25.10.2010 / 00:49
fonte
20

Conjunções nos nomes dos métodos:

public void addEmployeeAndUpdatePayrate(...) 


public int getSalaryOrHourlyPay(int Employee) ....

Esclarecimento: a razão pela qual isso soa os alarmes é que isso indica que o método provavelmente viola o princípio de responsabilidade única .

    
por 29.07.2017 / 04:43
fonte
13

Vinculando obviamente o código-fonte GPL em um programa comercial de código fechado.

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

    
por 04.08.2011 / 19:02
fonte
9

Idioma agnóstico:

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

Linguagem específica (C ++):

  • Macros C ++ em letras minúsculas.
  • Variáveis estáticas ou globais de C ++.
  • Variáveis não inicializadas ou não usadas.
  • Qualquer array new que aparentemente não é seguro para RAII.
  • Qualquer uso de matriz ou ponteiro que aparentemente não é seguro para limites. Isso inclui printf .
  • Qualquer uso da parte não inicializada de uma matriz.

Específico do Microsoft C ++:

  • Qualquer nome de identificador que colida 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 alarme. Sempre tenha o MSDN aberto e procure as definições de valor de argumentos / retorno sempre que estiver em dúvida. (editado)

C ++ / OOP específico:

  • Implementação (classe concreta) de herança em que a classe pai possui métodos virtuais e não virtuais, sem uma clara distinção conceitual óbvia entre o que deve / não deve ser virtual.
por 12.03.2012 / 18:27
fonte
8

Estilo de recuo 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é mesmo caseiro. Este é um sinal de que eles provavelmente não estão codificando com ninguém além deles mesmos.

    
por 27.10.2010 / 01:53
fonte
8

Usando muitos blocos de texto em vez de enums ou variáveis definidas globalmente.

Nada bom:

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

Melhor:

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

Melhor:

private itemTypeEnum itemType;
...
if (itemType == itemTypeEnum.Student) { ... }
    
por 29.07.2017 / 04:46
fonte
8

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

public DataTable GetEmployees() {...}

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

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}
    
por 29.07.2017 / 04:47
fonte
7
  • Vários operadores ternários são agrupados, então, em vez de parecer um bloco if...else , ele se torna um bloco if...else if...[...]...else
  • Nomes de variáveis longos sem sublinhados ou camelCasing. Exemplo de algum código que eu abri: $lesseeloginaccountservice
  • Centenas de linhas de código em um arquivo, com pouco ou nenhum comentário, e o código é muito não óbvio
  • Instruções if excessivamente complicadas. Exemplo do código: if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL)) , que reduzi para if ($lessee_obj == null)
por 25.10.2010 / 16:38
fonte
7

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

This sort of thing always worries me as there's that truism that everyone thinks they're an above average driver.

Aqui está um flash de notícias para você: 50% da população mundial está abaixo da média de inteligência. Ok, então algumas pessoas teriam exatamente inteligência média, mas não vamos ser 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.

Which things instantly ring alarm bells when looking at code?

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 um motivo. Muitas vezes isso pode ser subjetivo, mas na minha experiência eles são justificados. Seguir as melhores práticas não deve ser um problema, e se você está se perguntando por que elas são como são, pesquise em vez de ignorar e / ou reclamar sobre isso - talvez seja justificado, talvez não seja.

Um exemplo de uma prática recomendada pode estar usando curlies com todos os if-block, mesmo que contenha apenas uma linha:

if (something) {
    // whatever
}

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

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

    
por 29.07.2017 / 04:48
fonte
6

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

Isso acontece em um parágrafo inteiro.

Eles explicam que o seguinte refatorador precisa acontecer.

Mas não o fez.

Agora, eles poderiam ter sido informados de que não poderiam mudá-lo por seu chefe na época, por causa de questões de tempo ou competência, mas talvez fosse por causa de pessoas sendo mesquinhas.

Se um supervisor pensa que j.random. programador não pode fazer uma refatoração, então o supervisor deve fazer isso.

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 projetos de subsistemas borked.

História verdadeira. Pode acontecer com você.

    
por 25.10.2010 / 01:54
fonte
6

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

    
por 25.10.2010 / 16:02
fonte
6

Captura de exceções gerais:

try {

 ...

} catch {
}

ou

try {

 ...

} catch (Exception ex) {
}

Uso excessivo da região

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

    
por 29.07.2017 / 04:50
fonte
5

Convenções de nomenclatura de classe que demonstram um entendimento ruim 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 eu vi uma vez que foi intitulado Data e tinha mais de 30.000 linhas ... no arquivo primeiro . Era uma aula parcial dividida em pelo menos meia dúzia de outros arquivos. A maioria dos métodos eram wrappers em torno de procs armazenados com nomes como FindXByYWithZ() .

Mesmo com exemplos menos dramáticos, tenho certeza que todos nós acabamos de "despejar" a lógica em uma classe mal concebida, considerando-a um título totalmente genérico, e lamentamos isso mais tarde.

    
por 25.10.2010 / 16:17
fonte
5

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

    
por 18.12.2010 / 11:22
fonte
5
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...

Claro, sem qualquer tipo de documentação e os ocasionais aninhados #define s

    
por 29.07.2017 / 04:49
fonte