J'ai assisté à un événement sur la conception de logiciels il y a quelques semaines et l'un des commentaires formulés était: "Je suis sûr que nous reconnaissons tous le code défectueux quand nous le voyons" et tout le monde a acquiescé avec honte sans autre discussion.
Ce genre de chose m'inquiète toujours car il y a ce truisme que tout le monde pense être un conducteur supérieur à la moyenne. Bien que je pense pouvoir reconnaître le mauvais code, j'aimerais en savoir plus sur ce que les autres utilisateurs considèrent comme des odeurs de code, car il est rarement discuté en détail sur les blogs des internautes et seulement dans une poignée de livres. En particulier, je pense qu'il serait intéressant d'entendre parler de tout ce qui ressemble à une odeur de code dans une langue mais pas une autre.
Je vais commencer par un facile:
Code dans le contrôle de source qui contient une forte proportion de code commenté - pourquoi est-il là? Était-ce censé être supprimé? est-ce un travail à moitié fini? peut-être n'aurait-il pas dû être commenté et n'a été fait que lorsque quelqu'un testait quelque chose? Personnellement, je trouve ce genre de chose vraiment ennuyeux, même s’il s’agit d’une simple ligne ici et là, mais il est totalement inacceptable de voir de gros blocs entremêlés du reste du code. Cela indique aussi généralement que le reste du code sera probablement de qualité douteuse.
la source
printf("%c", 7)
sonne typiquement des cloches d’alarme pour moi. ;)Réponses:
Généralement trouvé dans un
try..catch
bloc absurde , il a tendance à attirer mon attention. À peu près aussi bien que/* Not sure what this does, but removing it breaks the build */
.Quelques autres choses:
if
déclarations complexes imbriquéesprocess
,data
,change
,rework
,modify
Un que je viens de trouver:
Oui, car le fait de forcer brutalement vos connexions MySQL est la bonne façon de faire les choses. Il s'est avéré que la base de données avait des problèmes avec le nombre de connexions, elle a donc continué à expirer. Au lieu de déboguer cela, ils ont simplement essayé encore et encore jusqu'à ce que cela fonctionne.
la source
Le principal drapeau rouge pour moi est le double des blocs de code, car il montre que la personne ne comprend pas les bases de la programmation ou a trop peur pour apporter les modifications appropriées à une base de code existante.
J'avais aussi l'habitude de compter le manque de commentaires comme un drapeau rouge, mais comme j'ai récemment travaillé sur beaucoup de très bons codes sans commentaires, je suis revenu à cette question.
la source
Code qui tente de montrer l’intelligence du programmeur, même s’il n’ajoute aucune valeur réelle:
la source
swap(x, y);
^_^
20 000 fonctions de ligne (exagérées). Toute fonction nécessitant plus que quelques écrans nécessite une refactorisation.
Dans le même ordre d'idées, les fichiers de classe qui semblent durer éternellement. Il existe probablement quelques concepts que l'on pourrait résumer en classes afin de clarifier le but et la fonction de la classe d'origine, et probablement leur utilisation, à moins qu'il s'agisse de méthodes internes.
variables non descriptives, non triviales, ou trop nombreuses variables non descriptives triviales. Celles-ci font de ce qui se passe réellement une énigme.
la source
Le pire, c'est que ça vient d'une bibliothèque commerciale!
la source
Les commentaires si verbeux que s'il y avait un compilateur anglais, celui-ci serait parfaitement compilé et parfaitement exécuté, sans toutefois décrire le code.
En outre, les commentaires sur le code qui auraient pu être supprimés s’ils avaient été respectés:
la source
/
de*/
est manquant, donc tout le code à la fin de la prochaine*/
est ignoré. Heureusement, la coloration syntaxique rend ce genre de chose rare de nos jours.a
pour user_age? Vraiment?i = i + 1; //increment i
Code qui produit des avertissements lors de la compilation.
la source
(unsigned int)
que d’encombrer mes listes d’avertissements / erreurs avec des avertissements bénins. Je détesterais que la liste d'avertissement devienne un angle mort. Il est aussi beaucoup plus d'un PITA expliquer à d' autres personnes pour lesquelles vous ne tenez pas compte d' un avertissement que d'expliquer pourquoi vous faites un casting de naturelints
àunsigned ints
.Fonctionne avec des nombres dans le nom au lieu d'avoir des noms descriptifs , comme:
Faites en sorte que les noms de fonctions aient un sens! Si doQething et doquething2 font des choses similaires, utilisez des noms de fonctions qui différencient les différences. Si doSomething2 est une rupture des fonctionnalités de doSomething, nommez-le pour ses fonctionnalités.
la source
mshtml
- ça me brise les yeux :(Chiffres magiques ou cordes magiques.
la source
200
, en revanche ...Peut-être pas le pire mais montre clairement le niveau des développeurs:
Si un langage a une construction for loop ou iterator, utiliser une boucle while montre également le niveau de compréhension du langage des développeurs
Une mauvaise orthographe / grammaire dans la documentation / commentaires me ronge presque autant que le code lui-même. La raison en est que le code était destiné aux humains à lire et aux machines à fonctionner. C’est la raison pour laquelle nous utilisons des langages de haut niveau. Si votre documentation est difficile à maîtriser, j’ai un avis négatif sur la base de code sans la regarder.
la source
Celui que je remarque immédiatement est la fréquence des blocs de code profondément imbriqués (if, while, etc). Si le code a souvent plus de deux ou trois niveaux de profondeur, c'est le signe d'un problème de conception / logique. Et s’il a une profondeur de 8 nids, il vaut mieux une bonne raison de ne pas le briser.
la source
return
énoncé, mais quand elle provoque plus de 6 niveaux d'imbrication si / alors, je pense que cela fait plus de mal que de bien.Lors de la notation du programme d'un étudiant, je peux parfois dire en un clin d'œil. Ce sont les indices instantanés:
Mes premières impressions sont rarement incorrectes et ces sonneries d’avertissement ont raison environ 95% du temps . À une exception près, un étudiant débutant dans la langue utilisait un style provenant d'un autre langage de programmation. Le fait de creuser et de relire leur style dans le langage de l’autre langue a permis de sonner l’alarme, et l’élève a eu tout le crédit qu’il méritait. Mais de telles exceptions sont rares.
Lorsque vous envisagez un code plus avancé, voici mes autres avertissements:
En termes de style, je n'aime généralement pas voir:
Ce ne sont que des indices sur le mauvais code. Parfois, ce qui peut sembler être du mauvais code ne l'est pas vraiment, car vous ne connaissez pas les intentions du programmeur. Par exemple, il peut y avoir une bonne raison pour que quelque chose semble trop complexe - il peut y avoir eu une autre considération en jeu.
la source
Favori personnel / bête noire: Les noms générés par l'IDE ont été validés. Si TextBox1 est une variable majeure et importante dans votre système, vous avez une autre chose à venir, la révision du code.
la source
Variables totalement inutilisées , en particulier lorsque la variable porte un nom similaire aux noms de variables utilisés.
la source
Beaucoup de gens ont mentionné:
Bien que je souhaite que cela soit mis en œuvre, au moins ils ont pris une note. Ce que je pense être pire, c'est:
Todo's sont sans valeur et déroutant si vous ne prenez jamais la peine de les enlever!
la source
//TODO
commentaire? Impressionnant!// TODO
utiliser votre traqueur de bogues, c’est la raison de son utilisation!Une méthode qui nécessite que je fasse défiler l'écran pour tout lire.
la source
Conjonctions dans les noms de méthodes:
Clarification: La raison pour laquelle cela sonne l'alarme est qu'elle indique que la méthode enfreint probablement le principe de responsabilité unique .
la source
addEmployee(); updatePayrate();
), alors je ne pense pas que ce soit un problème.Lier évidemment le code source de GPL à un programme commercial à source fermée.
Non seulement cela crée un problème juridique immédiat, mais selon mon expérience, cela indique généralement soit une négligence ou une insouciance, ce qui est également reflété ailleurs dans le code.
la source
Langue agnostique:
TODO: not implemented
int function(...) { return -1; }
(identique à "non implémenté")0
,-1
ounull
comme valeurs de retour exceptionnelles.Spécifique à la langue (C ++):
array new
qui n'est apparemment pas RAII-safe.printf
.Microsoft C ++ spécifique:
Spécifique à C ++ / OOP:
la source
Style d'indentation bizarre.
Il y a quelques styles très populaires, et les gens vont pousser ce débat dans la tombe. Mais parfois, je vois quelqu'un qui utilise un style d'indentation vraiment rare, voire développé localement. C'est un signe qu'ils n'ont probablement pas codé avec quelqu'un d'autre qu'eux-mêmes.
la source
Utiliser beaucoup de blocs de texte plutôt que des énumérations ou des variables définies globalement.
Pas bon:
Mieux:
Meilleur:
la source
Paramètres faiblement typés ou renvoyant des valeurs sur des méthodes.
la source
if...else
bloc, il devient unif...else if...[...]...else
bloc$lesseeloginaccountservice
if
. Exemple de code:if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL))
que j'ai chopé àif ($lessee_obj == null)
la source
Code odeur: ne pas suivre les meilleures pratiques
Voici une nouvelle pour vous: 50% de la population mondiale a une intelligence inférieure à la moyenne. Ok, alors certaines personnes auraient une intelligence tout à fait moyenne, mais ne soyons pas pointilleux. En outre, l'un des effets secondaires de la stupidité est que vous ne pouvez pas reconnaître votre propre stupidité! Les choses ne semblent pas si bonnes si vous combinez ces déclarations.
Beaucoup de bonnes choses ont été mentionnées et, en général, il semble que ne pas suivre les meilleures pratiques est une odeur de code.
Les meilleures pratiques ne sont généralement pas inventées au hasard et sont souvent là pour une raison. Plusieurs fois, cela peut être subjectif, mais d'après mon expérience, ils sont principalement justifiés. Suivre les meilleures pratiques ne devrait pas être un problème, et si vous vous demandez pourquoi elles sont ce qu’elles sont, faites des recherches plutôt que de les ignorer et / ou de vous en plaindre - peut-être que cela est justifié, peut-être que non.
Un exemple de meilleure pratique pourrait être d'utiliser des curlies avec chaque if-block, même s'il ne contient qu'une seule ligne:
Vous n’allez peut-être pas penser que c’est nécessaire, mais j’ai lu récemment que c’est une source majeure de bugs. Toujours utiliser des crochets a également été discuté sur Stack Overflow , et vérifier que les instructions if ont des crochets est également une règle dans PMD , un analyseur de code statique pour Java.
Rappelez-vous: "Parce que c'est la meilleure pratique" n'est jamais une réponse acceptable à la question "pourquoi faites-vous cela?" Si vous ne pouvez pas expliquer pourquoi quelque chose est une meilleure pratique, alors ce n'est pas une meilleure pratique, c'est une superstition.
la source
Les commentaires qui disent "c'est parce que la conception du sous-système froz est totalement bouleversée".
Cela va sur un paragraphe entier.
Ils expliquent que le refactor suivant doit se produire.
Mais ne l'a pas fait.
Maintenant, on leur aurait peut-être dit que leur patron ne pouvait pas le changer à l'époque, à cause de problèmes de temps ou de compétences, mais peut-être à cause de la petitesse des gens.
Si un superviseur pense que j.random. le programmeur ne peut pas refactoriser, alors le superviseur devrait le faire.
Quoi qu'il en soit, je sais que le code a été écrit par une équipe divisée, avec une politique de pouvoir possible, et ils n'ont pas refactorisé les conceptions de sous-système bouchées.
Histoire vraie. Ça pourrait t'arriver.
la source
Quelqu'un peut-il penser à un exemple où le code devrait légitimement faire référence à un fichier par chemin absolu?
la source
/dev/null
et les amis vont bien. Mais même des choses comme celles-ci/bin/bash
sont suspectes - et si vous êtes un système bizarre/usr/bin/bash
?/home/tom/dev/randomhacking/thing.wsdl
. Il est insensé sur le plan criminel qu'il s'agisse d'un comportement par défaut./dev/null
: J'ai l'habitude, lors du développement sur Windows, de conserver les applications et les bibliothèques sousc:\dev
. D'une manière ou d'une autre, un dossiernull
est toujours automatiquement créé dans ce dossier. Je jure que je n'ai aucune idée de qui fait ça. (Un de mes bugs préférés / fonctionnalités)Saisir les exceptions générales:
ou
Surutilisation de la région
Généralement, utiliser trop de régions signifie que vos classes sont trop grandes. C'est un drapeau d'avertissement qui indique que je devrais approfondir cette partie du code.
la source
Les conventions de dénomination de classe qui démontrent une faible compréhension de l'abstraction qu'elles tentent de créer. Ou qui ne définissent pas du tout une abstraction.
Un exemple extrême me vient à l’esprit dans une classe VB que j’ai vue une fois et qui portait un titre
Data
et qui comptait plus de 30 000 lignes… dans le premier fichier. C'était une classe partielle divisée en au moins une demi-douzaine d'autres fichiers. La plupart des méthodes étaient des wrappers entourant des procs avec des noms commeFindXByYWithZ()
.Même avec des exemples moins dramatiques, je suis sûr que nous avons tous simplement «jeté» la logique dans une classe mal conçue, en lui attribuant un titre totalement générique, que nous avons regretté par la suite.
la source
Fonctions qui réimplémentent les fonctionnalités de base du langage. Par exemple, si vous voyez une méthode "getStringLength ()" en JavaScript au lieu d'appeler la propriété ".length" d'une chaîne, vous savez que vous avez un problème.
la source
Bien sûr, sans aucun type de documentation et les
#define
s emboîtés occasionnelsla source