Aujourd’hui, nous discutons de la qualité du code C# et d’une variété d’erreurs par l’exemple du CMS DotNetNuke. Nous allons creuser dans son code source. Vous allez avoir besoin d’une tasse de café…
DotNetNuke
DotNetNuke est un système de gestion de contenu (CMS) open source écrit principalement en C#. Le code source est disponible sur GitHub. Le projet fait partie de la Fondation .NET.
Le projet a son site web, Twitter, Chaîne Youtube.
Cependant, je ne comprends toujours pas l’état du projet. Le référentiel GitHub est mis à jour de temps en temps. Ils ont de nouvelles versions. Cependant, cela fait un moment qu’ils n’ont pas posté quelque chose sur Twitter ou leur chaîne YouTube.
En même temps, ils ont un site Web communautaire où vous pouvez trouver des informations sur certains événements.
Quoi qu’il en soit, nous sommes particulièrement intéressés par le code. Le code et sa qualité.
Soit dit en passant, la page Web du projet (voir une capture d’écran ci-dessous) montre que les développeurs utilisent l’analyseur statique NDepend pour surveiller la qualité du code.
Je ne sais pas comment les développeurs du projet ont configuré l’analyseur, si les avertissements sont gérés, etc. Mais je voudrais vous rappeler qu’il est préférable d’utiliser régulièrement des outils d’analyse statique dans votre processus de développement. Vous pouvez trouver de nombreux articles sur ce sujet – visitez notre blog pour en lire quelques-uns.
À propos du chèque
Pour vérifier le projet, j’ai utilisé le code source de GitHub du 22 octobre 2021. Tenez compte du fait que nous avons publié / que vous avez lu cet article après un certain temps. Le code peut être différent maintenant.
J’ai utilisé PVS-Studio 7.15 pour effectuer l’analyse. Vous voulez essayer l’analyseur sur votre projet ? Cliquez ici pour ouvrir la page avec toutes les étapes nécessaires. Avez-vous des questions? Vous ne comprenez pas quelque chose ? N’hésitez pas à nous contacter.
Aujourd’hui, j’aimerais commencer par l’une des nouvelles fonctionnalités de PVS-Studio 7.15 – la meilleure liste d’avertissements. La fonctionnalité est toute nouvelle et nous l’améliorerons à l’avenir. Cependant, vous pouvez (et devez) l’utiliser dès maintenant.
Meilleurs avertissements
Disons que vous décidez d’essayer un analyseur statique sur votre projet. Vous l’avez téléchargé, analysé le projet et… vous avez reçu un tas d’avertissements. Des dizaines, des centaines, des milliers, voire des dizaines de milliers. Wow, « cool »… Ce serait génial de sélectionner comme par magie, par exemple, le Top 10 des avertissements les plus intéressants. Assez pour regarder et penser: « Ouais, ce code est de la foutaise, définitivement! ». Eh bien, maintenant PVS-Studio dispose d’un tel mécanisme. C’est ce qu’on appelle les meilleurs avertissements.
Jusqu’à présent, vous ne pouvez utiliser la fonctionnalité que dans le plug-in PVS-Studio pour Visual Studio. Mais nous prévoyons d’ajouter les meilleurs avertissements aux autres plugins IDE plus tard. Avec le meilleur mécanisme d’avertissements, l’analyseur sélectionne les avertissements les plus intéressants et plausibles dans le journal.
Prêt à voir la meilleure liste d’avertissements pour le projet DNN ?
Meilleurs avertissements. Numéro 1
public string NavigateURL(int tabID,
bool isSuperTab,
IPortalSettings settings,
....)
{
....
if (isSuperTab)
{
url += "&portalid=" + settings.PortalId;
}
TabInfo tab = null;
if (settings != null)
{
tab = TabController.Instance.GetTab(tabID,
isSuperTab ? Null.NullInteger : settings.PortalId, false);
}
....
}L’avertissement PVS-Studio : V3095 L’objet ‘settings’ a été utilisé avant d’être vérifié par rapport à null. Vérifier les lignes : 190, 195. DotNetNuke.Library NavigationManager.cs 190
Je me demande pourquoi au début on accède au settings.PortalId propriété d’instance, puis nous vérifions Les paramètres pour nul inégalité. Ainsi, si Les paramètres – nul et isSuperTab – vrai, on a NullReferenceException.
Étonnamment, ce fragment de code a un deuxième contrat qui lie isSuperTab et Les paramètres paramètres – l’opérateur ternaire : estSuperTab ? Null.NullInteger : settings.PortalId. Notez que dans ce cas, contrairement si, settings.PortalId est utilisé lorsque isSuperTab est faux.
Si isSuperTab est vrai, les settings.PortalId la valeur n’est pas traitée. Vous pouvez penser que c’est juste un contrat implicite, et tout va bien.
Nan.
Le code doit être facile à lire et compréhensible – vous n’avez pas à penser comme Sherlock. Si vous avez l’intention de créer ce contrat, écrivez-le explicitement dans le code. Ainsi, les développeurs, l’analyseur statique et vous ne serez pas déroutés. 😉
Meilleurs avertissements. Numéro 2
private static string GetTableName(Type objType)
{
string tableName = string.Empty;
// If no attrubute then use Type Name
if (string.IsNullOrEmpty(tableName))
{
tableName = objType.Name;
if (tableName.EndsWith("Info"))
{
// Remove Info ending
tableName.Replace("Info", string.Empty);
}
}
....
}L’avertissement PVS-Studio : V3010 La valeur de retour de la fonction ‘Remplacer’ doit être utilisée. DotNetNuke.Library CBO.cs 1038
Nous avons ici plusieurs cas curieux :
- les développeurs voulaient supprimer le « Info » sous-chaîne de nom de la table mais j’ai oublié que les chaînes C# sont immuables. nom de la table reste le même. La chaîne remplacée est perdue, car le résultat de la Remplacer l’appel de méthode n’est stocké nulle part ;
- les nom de la table variable initialisée avec une chaîne vide est déclarée dans le code. Juste après, les développeurs vérifient si nom de la table est une chaîne vide.
L’analyseur émet l’avertissement pour le premier cas. Soit dit en passant, l’analyseur détecte également le deuxième cas. Cependant, la liste des meilleurs avertissements n’inclut pas cet avertissement. Le voici : V3022 L’expression ‘string.IsNullOrEmpty(tableName)’ est toujours vraie. DotNetNuke.Library CBO.cs 1032
Meilleurs avertissements. Numéro 3
public static ArrayList GetFileList(...., string strExtensions, ....)
{
....
if ( strExtensions.IndexOf(
strExtension,
StringComparison.InvariantCultureIgnoreCase) != -1
|| string.IsNullOrEmpty(strExtensions))
{
arrFileList.Add(new FileItem(fileName, fileName));
}
....
}L’avertissement PVS-Studio : V3027 La variable ‘strExtensions’ a été utilisée dans l’expression logique avant d’être vérifiée par rapport à null dans la même expression logique. DotNetNuke.Library Globals.cs 3783
Dans le strExtensions chaîne, les développeurs essaient de trouver la strExtension sous-chaîne. Si la sous-chaîne n’est pas trouvée, ils vérifient si strExtensions est vide ou nul. Mais si strExtensions est nul, les Indice de appel mène à NullReferenceException.
Si strExtension est implicitement une chaîne vide mais n’a jamais de nul valeur, on peut exprimer plus explicitement les intentions : strExtensions.Length == 0.
Dans tous les cas, il vaut mieux corriger ce fragment de code car il soulève des questions – comme dans Numéro 1.
Meilleurs avertissements. Numéro 4
public static void KeepAlive(Page page)
{
....
var scriptBlock = string.Format(
"(function($){{setInterval(
function(){{$.get(location.href)}}, {1});}}(jQuery));",
Globals.ApplicationPath,
seconds);
....
}L’avertissement PVS-Studio : V3025 Format incorrect. Un nombre différent d’éléments de format est attendu lors de l’appel de la fonction ‘Format’. Arguments non utilisés : Globals.ApplicationPath. DotNetNuke.Library jQuery.cs 402
Opérations suspectes avec des chaînes formatées – la valeur du secondes variable est substituée dans la chaîne résultante. Mais il n’y avait pas de place pour Globals.ApplicationPath en raison de l’absence de {0} dans la chaîne de format.
Meilleurs avertissements. Numéro 5
private void ProcessRequest(....)
{
....
if (!result.RewritePath.ToLowerInvariant().Contains("tabId="))
....
}L’avertissement PVS-Studio : V3122 La chaîne en minuscules ‘result.RewritePath.ToLowerInvariant()’ est comparée à la chaîne en casse mixte ‘ »tabId= »‘. DotNetNuke.Library AdvancedUrlRewriter.cs 2252
Je suppose que je n’ai jamais vu d’avertissements de ce diagnostic dans les projets. Eh bien, une première fois pour tout. 🙂
Les développeurs mettent en minuscule la chaîne de Chemin de réécriture et vérifiez s’il a le « tabId = » sous-chaîne. Mais il y a un problème – la chaîne source est en minuscule, mais la chaîne qu’ils vérifient contient des caractères majuscules.
Meilleurs avertissements. Numéro 6
protected override void RenderEditMode(HtmlTextWriter writer)
{
....
// Add the Not Specified Option
if (this.ValueField == ListBoundField.Text)
{
writer.AddAttribute(HtmlTextWriterAttribute.Value, Null.NullString);
}
else
{
writer.AddAttribute(HtmlTextWriterAttribute.Value, Null.NullString);
}
....
}L’avertissement PVS-Studio : V3004 L’instruction ‘then’ est équivalente à l’instruction ‘else’. DotNetNuke.Library DNNListEditControl.cs 380
Copier-coller classique : alors et autre succursales de la si déclaration sont identiques.
Meilleurs avertissements. Numéro 7
public static string LocalResourceDirectory
{
get
{
return "App_LocalResources";
}
}
private static bool HasLocalResources(string path)
{
var folderInfo = new DirectoryInfo(path);
if (path.ToLowerInvariant().EndsWith(Localization.LocalResourceDirectory))
{
return true;
}
....
}L’avertissement PVS-Studio : V3122 La chaîne en minuscules ‘path.ToLowerInvariant()’ est comparée à la chaîne en casse mixte ‘Localization.LocalResourceDirectory’. Dnn.PersonaBar.Extensions LanguagesController.cs 644
On y va encore une fois. Mais cette fois, l’erreur est moins évidente. Les développeurs convertissent le chemin valeur en minuscule. Ensuite, ils vérifient si cela se termine par une chaîne contenant des caractères majuscules – « App_LocalResources » (le littéral renvoyé du LocalResourceDirectory biens).
Meilleurs avertissements. Numéro 8
internal static IEnumerable<PropertyInfo> GetEditorConfigProperties()
{
return
typeof(EditorConfig).GetProperties()
.Where(
info => !info.Name.Equals("Magicline_KeystrokeNext")
&& !info.Name.Equals("Magicline_KeystrokePrevious")
&& !info.Name.Equals("Plugins")
&& !info.Name.Equals("Codemirror_Theme")
&& !info.Name.Equals("Width")
&& !info.Name.Equals("Height")
&& !info.Name.Equals("ContentsCss")
&& !info.Name.Equals("Templates_Files")
&& !info.Name.Equals("CustomConfig")
&& !info.Name.Equals("Skin")
&& !info.Name.Equals("Templates_Files")
&& !info.Name.Equals("Toolbar")
&& !info.Name.Equals("Language")
&& !info.Name.Equals("FileBrowserWindowWidth")
&& !info.Name.Equals("FileBrowserWindowHeight")
&& !info.Name.Equals("FileBrowserWindowWidth")
&& !info.Name.Equals("FileBrowserWindowHeight")
&& !info.Name.Equals("FileBrowserUploadUrl")
&& !info.Name.Equals("FileBrowserImageUploadUrl")
&& !info.Name.Equals("FilebrowserImageBrowseLinkUrl")
&& !info.Name.Equals("FileBrowserImageBrowseUrl")
&& !info.Name.Equals("FileBrowserFlashUploadUrl")
&& !info.Name.Equals("FileBrowserFlashBrowseUrl")
&& !info.Name.Equals("FileBrowserBrowseUrl")
&& !info.Name.Equals("DefaultLinkProtocol"));
}L’avertissement PVS-Studio : V3001 Il y a des sous-expressions identiques ‘!info.Name.Equals(« Templates_Files »)’ à gauche et à droite de l’opérateur ‘&&’. DNNConnect.CKEditorProvider SettingsUtil.cs 1451
J’ai formaté ce code pour qu’il soit plus clair. L’analyseur a détecté un doublon suspect de chèques : !info.Nom.Equals(« Modèles_Fichiers »). Peut-être que ce code est redondant. Ou un chèque nécessaire s’est perdu ici.
En fait, nous avons également d’autres doublons ici. Pour une raison quelconque, l’analyseur n’a pas signalé à leur sujet (nous le vérifierons plus tard). De plus, les expressions suivantes apparaissent deux fois :
- !info.Name.Equals(« FileBrowserWindowWidth »)
- !info.Nom.Equals(« FileBrowserWindowHeight »)
Trois chèques en double dans le…