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

/* 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.

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.

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

x ^= y ^= x ^= y;

• 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.

{ Let it Leak, people have good enough computers to cope these days }

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

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;

Código que produz avisos quando compilado.

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.

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

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

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }

• 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.

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.

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.

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.

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

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!

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

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 .

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.

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.

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.

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) { ... }

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

public DataTable GetEmployees() {...}

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

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}

• 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)

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.

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ê.

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

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.

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.

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.

#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