Today, we discuss C# code quality and a variety of errors by the example of CMS DotNetNuke. We're going to dig into its source code. You're going to need a cup of coffee...
DotNetNuke is an open-source content management system (CMS) written mainly in C#. The source code is available on GitHub. The project is part of the .NET Foundation.
The project has its website, Twitter, YouTube channel.
However, I still don't understand the project status. The GitHub repository is updated from time to time. They have new releases. Though, it's been a while since they post something on Twitter or YouTube channel.
At the same time, they have a community website where you can find information about some events.
Anyway, we are interested in the code especially. The code and its quality.
By the way, the project web page (see a screenshot below) shows that the developers use the NDepend static analyzer to monitor code quality.
I don't know how the project developers configured the analyzer, whether warnings are handled, and so on. But I'd like to remind you that it's better to use static analysis tools regularly in your development process. You can find lots of articles on this topic – visit our blog to read some.
To check the project, I used the source code from GitHub from the 22nd of October 2021. Take into account that we published / you read this article after a while. The code may be different by now.
I use PVS-Studio 7.15 to perform the analysis. Want to try the analyzer on your project? Click here to open the page with all the necessary steps. Have any questions? Don't understand something? Feel free to contact us.
Today, I'd like to start with one of the new features of PVS-Studio 7.15 – the best warnings list. The feature is brand-new, and we will enhance it in the future. However, you can (and should) use it right now.
Let's say you decide to try a static analyzer on your project. You downloaded it, analyzed the project, and... got a bunch of warnings. Tens, hundreds, thousands, maybe even tens of thousands. Wow, "cool"... It would be great to magically select, for example, the Top 10 most interesting warnings. Enough to look at and think: "Yeah, that code is rubbish, definitely!". Well, now PVS-Studio has such a mechanism. It is called best warnings.
So far, you can use the feature only in the PVS-Studio plugin for Visual Studio. But we plan to add the best warnings to other IDE plugins later. With the best warnings mechanism, the analyzer selects the most interesting and plausible warnings from the log.
Ready to see the best warnings list for the DNN project?
Best warnings. Issue 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);
}
....
}The PVS-Studio warning: V3095 The 'settings' object was used before it was verified against null. Check lines: 190, 195. DotNetNuke.Library NavigationManager.cs 190
I wonder why at first we access the settings.PortalId instance property, and then we check settings for null inequality. Thus, if settings – null and isSuperTab – true, we get NullReferenceException.
Surprisingly, this code fragment has a second contract that links isSuperTab and settings parameters – the ternary operator: isSuperTab ? Null.NullInteger : settings.PortalId. Note that in this case, unlike if, settings.PortalId is used when isSuperTab is false.
If isSuperTab is true, the settings.PortalId value is not processed. You may think that it's just an implicit contract, and everything is fine.
Nope.
The code must be easy to read and understandable – you don't have to think like Sherlock. If you intend to create this contract, write it explicitly in the code. Thus, the developers, the static analyzer, and you will not be confused. ;)
Best warnings. Issue 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);
}
}
....
}The PVS-Studio warning: V3010 The return value of function 'Replace' is required to be utilized. DotNetNuke.Library CBO.cs 1038
Here we have several curious cases:
The analyzer issues the warning for the first case. By the way, the analyzer also detects the second case. However, the best warnings list does not include this warning. Here it is: V3022 Expression 'string.IsNullOrEmpty(tableName)' is always true. DotNetNuke.Library CBO.cs 1032
Best warnings. Issue 3
public static ArrayList GetFileList(...., string strExtensions, ....)
{
....
if ( strExtensions.IndexOf(
strExtension,
StringComparison.InvariantCultureIgnoreCase) != -1
|| string.IsNullOrEmpty(strExtensions))
{
arrFileList.Add(new FileItem(fileName, fileName));
}
....
}The PVS-Studio warning: V3027 The variable 'strExtensions' was utilized in the logical expression before it was verified against null in the same logical expression. DotNetNuke.Library Globals.cs 3783
In the strExtensions string, the developers try to find the strExtension substring. If the substring is not found, they check whether strExtensions is empty or null. But if strExtensions is null, the IndexOf call leads to NullReferenceException.
If strExtension is implied to be an empty string but never has a null value, we can more explicitly express the intentions: strExtensions.Length == 0.
In any case, it's better to fix this code fragment because it raises questions – as in Issue 1.
Best warnings. Issue 4
public static void KeepAlive(Page page)
{
....
var scriptBlock = string.Format(
"(function($){{setInterval(
function(){{$.get(location.href)}}, {1});}}(jQuery));",
Globals.ApplicationPath,
seconds);
....
}The PVS-Studio warning: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: Globals.ApplicationPath. DotNetNuke.Library jQuery.cs 402
Suspicious operations with formatted strings – the value of the seconds variable is substituted into the resulting string. But there was no place for Globals.ApplicationPath due to the absence of {0} in the format string.
Best warnings. Issue 5
private void ProcessRequest(....)
{
....
if (!result.RewritePath.ToLowerInvariant().Contains("tabId="))
....
}The PVS-Studio warning: V3122 The 'result.RewritePath.ToLowerInvariant()' lowercase string is compared with the '"tabId="' mixed case string. DotNetNuke.Library AdvancedUrlRewriter.cs 2252
Guess I've never seen warnings of this diagnostic in projects. Well, a first time for everything. :)
The developers lowercase the string from RewritePath and check whether it has the "tabId=" substring. But there's a problem – the source string is lowercased, but the string that they check contains uppercase characters.
Best warnings. Issue 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);
}
....
}The PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. DotNetNuke.Library DNNListEditControl.cs 380
Classic copy-paste: then and else branches of the if statement are identical.
Best warnings. Issue 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;
}
....
}The PVS-Studio warning: V3122 The 'path.ToLowerInvariant()' lowercase string is compared with the 'Localization.LocalResourceDirectory' mixed case string. Dnn.PersonaBar.Extensions LanguagesController.cs 644
Here we go again. But this time, the error is less obvious. The developers convert the path value to lowercase. Then, they check whether it ends in a string that contains uppercase characters – "App_LocalResources" (the literal returned from the LocalResourceDirectory property).
Best warnings. Issue 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"));
}The PVS-Studio warning: V3001 There are identical sub-expressions '!info.Name.Equals("Templates_Files")' to the left and to the right of the '&&' operator. DNNConnect.CKEditorProvider SettingsUtil.cs 1451
I've formatted this code to make it clearer. The analyzer detected a suspicious duplicate of checks: !info.Name.Equals("Templates_Files"). Perhaps this code is redundant. Or some necessary check got lost here.
In fact, we also have other duplicates here. For some reason, the analyzer did not report about them (we'll check it later). Also, the following expressions occur twice:
Three duplicate checks within the same expression – not bad. I guess that's a record!
Best warnings. Issue 9
private void ProcessContentPane()
{
....
string moduleEditRoles
= this.ModuleConfiguration.ModulePermissions.ToString("EDIT");
....
moduleEditRoles
= moduleEditRoles.Replace(";", string.Empty).Trim().ToLowerInvariant();
....
if ( viewRoles.Equals(this.PortalSettings.AdministratorRoleName,
StringComparison.InvariantCultureIgnoreCase)
&& (moduleEditRoles.Equals(this.PortalSettings.AdministratorRoleName,
StringComparison.InvariantCultureIgnoreCase)
|| string.IsNullOrEmpty(moduleEditRoles))
&& pageEditRoles.Equals(this.PortalSettings.AdministratorRoleName,
StringComparison.InvariantCultureIgnoreCase))
{
adminMessage = Localization.GetString("ModuleVisibleAdministrator.Text");
showMessage = !this.ModuleConfiguration.HideAdminBorder
&& !Globals.IsAdminControl();
}
....
}The PVS-Studio warning: V3027 The variable 'moduleEditRoles' was utilized in the logical expression before it was verified against null in the same logical expression. DotNetNuke.Library Container.cs 273
Hmm, too much code... Let's reduce it.
moduleEditRoles.Equals(this.PortalSettings.AdministratorRoleName,
StringComparison.InvariantCultureIgnoreCase)
|| string.IsNullOrEmpty(moduleEditRoles)So much better now! I guess we've already discussed something similar today... Again, at first, the developers check whether moduleEditRoles equals another string. Then they check whether moduleEditRoles is an empty string or a null value.
However, at this stage, the variable cannot store a null value because it contains the result of the ToLowerInvariant method. Therefore, it can be an empty string at most. We could lower the warning level of the analyzer here.
Though, I would fix the code by moving the IsNullOrEmpty check in the beginning.
Best warnings. Issue 10
private static void Handle404OrException(....)
{
....
string errRV;
....
if (result != null && result.Action != ActionType.Output404)
{
....
// line 552
errRV = "500 Rewritten to {0} : {1}";
}
else // output 404 error
{
....
// line 593
errRV = "404 Rewritten to {0} : {1} : Reason {2}";
....
}
....
// line 623
response.AppendHeader(errRH,
string.Format(
errRV,
"DNN Tab",
errTab.TabName
+ "(Tabid:" + errTabId.ToString() + ")",
reason));
....
}The PVS-Studio warning: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: reason. DotNetNuke.Library AdvancedUrlRewriter.cs 623
False positive. Obviously, the programmer intended to write the code this way. So, we must fix this at the analyzer level.
Summary
Not bad, I guess! Yeah, we have 1 false positive. But other issues in the code must be fixed.
However, you are free to create your list of the best warnings. For that, I describe other warnings below. :)
As you see, that's not all we have today! The analyzer found lots of worthy cases to consider.
Issue 11
In the best warnings section, we've already discussed a copy-paste of then/else branches of the if statement. Unfortunately, this is not the only place:
protected void ExecuteSearch(string searchText, string searchType)
{
....
if (Host.UseFriendlyUrls)
{
this.Response.Redirect(this._navigationManager.NavigateURL(searchTabId));
}
else
{
this.Response.Redirect(this._navigationManager.NavigateURL(searchTabId));
}
....
}The PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. DotNetNuke.Website Search.ascx.cs 432
Issues 12, 13
private static void LoadProviders()
{
....
foreach (KeyValuePair<string, SitemapProvider> comp in
ComponentFactory.GetComponents<SitemapProvider>())
{
comp.Value.Name = comp.Key;
comp.Value.Description = comp.Value.Description;
_providers.Add(comp.Value);
}
....
}The PVS-Studio warning: V3005 The 'comp.Value.Description' variable is assigned to itself. DotNetNuke.Library SitemapBuilder.cs 231
Sometimes you can encounter the code where a variable is assigned to itself. This code can be redundant or may contain a more serious error – perhaps the developers mixed something up. I guess the above code fragment is exactly the case.
Description is an auto-implemented property:
public string Description { get; set; }Here's one more fragment that contains the variable assigned to itself:
public SendTokenizedBulkEmail(List<string> addressedRoles,
List<UserInfo> addressedUsers,
bool removeDuplicates,
string subject,
string body)
{
this.ReportRecipients = true;
this.AddressMethod = AddressMethods.Send_TO;
this.BodyFormat = MailFormat.Text;
this.Priority = MailPriority.Normal;
this._addressedRoles = addressedRoles;
this._addressedUsers = addressedUsers;
this.RemoveDuplicates = removeDuplicates;
this.Subject = subject;
this.Body = body;
this.SuppressTokenReplace = this.SuppressTokenReplace;
this.Initialize();
}The PVS-Studio warning: V3005 The 'this.SuppressTokenReplace' variable is assigned to itself. DotNetNuke.Library SendTokenizedBulkEmail.cs 109
This code is not as suspicious as the previous one but still looks strange. The SuppressTokenReplace property is assigned to itself. The corresponding parameter is absent. I don't know what value must be assigned. Maybe the default value described in the comments (that is, false):
/// <summary>Gets or sets a value indicating whether
shall automatic TokenReplace be prohibited?.</summary>
/// <remarks>default value: false.</remarks>
public bool SuppressTokenReplace { get; set; }Issues 14, 15
In the best warnings section, we discussed that the developers forgot about strings' immutability. Well, they forgot about it more than once. :)
public static string BuildPermissions(IList Permissions, string PermissionKey)
{
....
// get string
string permissionsString = permissionsBuilder.ToString();
// ensure leading delimiter
if (!permissionsString.StartsWith(";"))
{
permissionsString.Insert(0, ";");
}
....
}The PVS-Studio warning: V3010 The return value of function 'Insert' is required to be utilized. DotNetNuke.Library PermissionController.cs 64
If permissionsString does not start with ';', the developers want to fix this by adding ';' in the beginning. However, Insert does not change the source string, it returns the modified one.
Another case:
public override void Install()
{
....
skinFile.Replace(Globals.HostMapPath + "\\", "[G]");
....
}The PVS-Studio warning: V3010 The return value of function 'Replace' is required to be utilized. DotNetNuke.Library SkinInstaller.cs 230
Issue 16
public int Page { get; set; } = 1;
public override IConsoleResultModel Run()
{
....
var pageIndex = (this.Page > 0 ? this.Page - 1 : 0);
pageIndex = pageIndex < 0 ? 0 : pageIndex;
....
}The PVS-Studio warning: V3022 Expression 'pageIndex < 0' is always false. DotNetNuke.Library ListModules.cs 61
When the pageIndex < 0 expression is evaluated, the pageIndex value will be always non-negative, since:
Therefore, the pageIndex < 0 check will always be false.
Issue 17
private CacheDependency GetTabsCacheDependency(IEnumerable<int> portalIds)
{
....
// get the portals list dependency
var portalKeys = new List<string>();
if (portalKeys.Count > 0)
{
keys.AddRange(portalKeys);
}
....
}The PVS-Studio warning: V3022 Expression 'portalKeys.Count > 0' is always false. DotNetNuke.Library CacheController.cs 968
The developers created an empty list and then checked that it is non-empty. Just in case :)
Issue 18
public JournalEntity(string entityXML)
{
....
XmlDocument xDoc = new XmlDocument { XmlResolver = null };
xDoc.LoadXml(entityXML);
if (xDoc != null)
....
}The PVS-Studio warning: V3022 Expression 'xDoc != null' is always true. DotNetNuke.Library JournalEntity.cs 30
Called the constructor, wrote the reference to a variable. After that, called the LoadXml instance method. Then, the developers check the same link for null inequality. Just in case. (2)
Issue 19
public enum ActionType
{
....
Redirect302Now = 2,
....
Redirect302 = 5,
....
}
public ActionType Action { get; set; }
private static bool CheckForRedirects(....)
{
....
if ( result.Action != ActionType.Redirect302Now
|| result.Action != ActionType.Redirect302)
....
}The PVS-Studio warning: V3022 Expression is always true. Probably the '&&' operator should be used here. DotNetNuke.Library AdvancedUrlRewriter.cs 1695
This expression will be false only if the result of both operands is false. In this case, the following conditions must be met:
Since result.Action cannot have two different values, the described condition is impossible. Therefore, the expression is always true.
Issue 20
public Route MapRoute(string moduleFolderName,
string routeName,
string url,
object defaults,
object constraints,
string[] namespaces)
{
if ( namespaces == null
|| namespaces.Length == 0
|| string.IsNullOrEmpty(namespaces[0]))
{
throw new ArgumentException(Localization.GetExceptionMessage(
"ArgumentCannotBeNullOrEmpty",
"The argument '{0}' cannot be null or empty.",
"namespaces"));
}
Requires.NotNullOrEmpty("moduleFolderName", moduleFolderName);
url = url.Trim('/', '\\');
var prefixCounts = this.portalAliasMvcRouteManager.GetRoutePrefixCounts();
Route route = null;
if (url == null)
{
throw new ArgumentNullException(nameof(url));
}
....
}The PVS-Studio warning: V3022 Expression 'url == null' is always false. DotNetNuke.Web.Mvc MvcRoutingManager.cs 66
What a curious case we have with the url parameter. If url is null, the developers want to throw ArgumentNullException. The exception unambiguously hints that this parameter should be non-null. But before this, for url, the developers call an instance method – Trim... As a result, if url is null, NullReferenceException is thrown.
Issue 21
public Hashtable Settings
{
get
{
return this.ModuleContext.Settings;
}
}
public string UploadRoles
{
get
{
....
if (Convert.ToString(this.Settings["uploadroles"]) != null)
{
this._UploadRoles = Convert.ToString(this.Settings["uploadroles"]);
}
....
}
}The PVS-Studio warning: V3022 Expression 'Convert.ToString(this.Settings["uploadroles"]) != null' is always true. DotNetNuke.Website.Deprecated WebUpload.ascx.cs 151
Convert.ToString may return the result of a successful converting or String.Empty, but not null. Eventually, this check makes no sense.
Believed it? This is false positive.
Let's start with the Convert.ToString method overloading: Convert.ToString(String value). It returns value as is. Thus, if the input is null, the output is also null.
The above code snippet contains another overloading – Convert.ToString(Object value). The return value of this method has the following comment:
// Returns:
// The string representation of value,
// or System.String.Empty if value is null.You may think that the method will always return some string. However, the string representation of the object may have a null value. As a result, the method will return null.
Here's the simplest example:
By the way, it turns out that:
Hmm, that's a bit tangled...
You could say that this is a synthetic example. Who returns null from the ToString method? Well, I know that Microsoft had a few cases (follow the link and take a look at Issue 14).
And here's the question! Did the code authors know about this peculiarity? Did they take this into account or not? What about you? Did you know about this?
By the way, nullable reference types can help here. The method's signature indicates that the method may return the null value. As a result, possible misunderstanding is gone:
public static string? ToString(object? value)Now it's time for a break. Pour some more coffee and take a few cookies. It's coffee break!
Grabbed a snack? We're proceeding to the next issue.
Issues 22, 23
public static ModuleItem ConvertToModuleItem(ModuleInfo module)
=> new ModuleItem
{
Id = module.ModuleID,
Title = module.ModuleTitle,
FriendlyName = module.DesktopModule.FriendlyName,
EditContentUrl = GetModuleEditContentUrl(module),
EditSettingUrl = GetModuleEditSettingUrl(module),
IsPortable = module.DesktopModule?.IsPortable,
AllTabs = module.AllTabs,
};The PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'module.DesktopModule' object Dnn.PersonaBar.Extensions Converters.cs 67
Take a look at FriendlyName and IsPortable initialization. The developers use module.DesktopModule.FriendlyName and module.DesktopModule?.IsPortable as values for initialization. You might ask – can module.DesktopModule be null? If it is null, ?. won't protect the code because module.DesktopModule.FriendlyName does not contain null checking. If it is not null, ?. is redundant and misleading.
Here's a strikingly similar code fragment.
public IDictionary<string, object> GetSettings(MenuItem menuItem)
{
var settings = new Dictionary<string, object>
{
{ "canSeePagesList",
this.securityService.CanViewPageList(menuItem.MenuId) },
{ "portalName",
PortalSettings.Current.PortalName },
{ "currentPagePermissions",
this.securityService.GetCurrentPagePermissions() },
{ "currentPageName",
PortalSettings.Current?.ActiveTab?.TabName },
{ "productSKU",
DotNetNukeContext.Current.Application.SKU },
{ "isAdmin",
this.securityService.IsPageAdminUser() },
{ "currentParentHasChildren",
PortalSettings.Current?.ActiveTab?.HasChildren },
{ "isAdminHostSystemPage",
this.securityService.IsAdminHostSystemPage() },
};
return settings;
}The PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'PortalSettings.Current' object Dnn.PersonaBar.Extensions PagesMenuController.cs 47
The same happens here. When developers initialize the dictionary, they use PortalSettings.Current several times. In some cases, they check it for null, in other cases, they don't:
var settings = new Dictionary<string, object>
{
....
{ "portalName",
PortalSettings.Current.PortalName },
....
{ "currentPageName",
PortalSettings.Current?.ActiveTab?.TabName },
....
{ "currentParentHasChildren",
PortalSettings.Current?.ActiveTab?.HasChildren },
....
};Issues 24, 25, 26
private static void HydrateObject(object hydratedObject, IDataReader dr)
{
....
// Get the Data Value's type
objDataType = coloumnValue.GetType();
if (coloumnValue == null || coloumnValue == DBNull.Value)
{
// set property value to Null
objPropertyInfo.SetValue(hydratedObject,
Null.SetNull(objPropertyInfo),
null);
}
....
}The PVS-Studio warning: V3095 The 'coloumnValue' object was used before it was verified against null. Check lines: 902, 903. DotNetNuke.Library CBO.cs 902
The GetType method is called for the coloumnValue variable. Then, coloumnValue != null is checked. This looks strange.
Unfortunately, we have another similar case. Here it is:
private void DeleteLanguage()
{
....
// Attempt to get the Locale
Locale language = LocaleController.Instance
.GetLocale(tempLanguagePack.LanguageID);
if (tempLanguagePack != null)
{
LanguagePackController.DeleteLanguagePack(tempLanguagePack);
}
....
}The PVS-Studio warning: V3095 The 'tempLanguagePack' object was used before it was verified against null. Check lines: 235, 236. DotNetNuke.Library LanguageInstaller.cs 235
The same story – at first, the LanguageId property (tempLanguagePack.LanguageID) is accessed. On the next line, the tempLanguagePack != null is checked.
And more...
private static void AddLanguageHttpAlias(int portalId, Locale locale)
{
....
var portalAliasInfos = portalAliasses as IList<PortalAliasInfo>
?? portalAliasses.ToList();
if (portalAliasses != null && portalAliasInfos.Any())
....
}The PVS-Studio warning: V3095 The 'portalAliasses' object was used before it was verified against null. Check lines: 1834, 1835. DotNetNuke.Library Localization.cs 1834
That's all for this pattern. Although, the analyzer issued similar warnings for other code fragments. Let's take a look at another way to refer to members before checking for null.
Issues 27, 28, 29, 30
private static void WatcherOnChanged(object sender, FileSystemEventArgs e)
{
if (Logger.IsInfoEnabled && !e.FullPath.EndsWith(".log.resources"))
{
Logger.Info($"Watcher Activity: {e.ChangeType}. Path: {e.FullPath}");
}
if ( _handleShutdowns
&& !_shutdownInprogress
&& (e.FullPath ?? string.Empty)
.StartsWith(_binFolder,
StringComparison.InvariantCultureIgnoreCase))
{
ShceduleShutdown();
}
}The PVS-Studio warning: V3095 The 'e.FullPath' object was used before it was verified against null. Check lines: 147, 152. DotNetNuke.Web DotNetNukeShutdownOverload.cs 147
Notice e.FullPath. At first, e.FullPath.EndsWith(".log.resources") is accessed. Then, the ?? operator is used: e.FullPath ?? string.Empty.
This code is successfully multiplied via copy-paste:
I think that's enough for V3095. And I guess you don't want to read about it anymore. So, let's move on.
Issue 31
internal FolderInfoBuilder()
{
this.portalId = Constants.CONTENT_ValidPortalId;
this.folderPath = Constants.FOLDER_ValidFolderRelativePath;
this.physicalPath = Constants.FOLDER_ValidFolderPath;
this.folderMappingID = Constants.FOLDER_ValidFolderMappingID;
this.folderId = Constants.FOLDER_ValidFolderId;
this.physicalPath = string.Empty;
}The PVS-Studio warning: V3008 The 'this.physicalPath' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 29, 26. DotNetNuke.Tests.Core FolderInfoBuilder.cs 29
The Constants.FOLDER_ValidFolderPath value is initially written in the physicalPath field. Then, string.Empty is assigned to the same field. Note that these values are different. That's why this code looks even more suspicious:
public const string FOLDER_ValidFolderPath = "C:\\folder";Issue 32
public int SeekCountry(int offset, long ipNum, short depth)
{
....
var buffer = new byte[6];
byte y;
....
if (y < 0)
{
y = Convert.ToByte(y + 256);
}
....
}The PVS-Studio warning: V3022 Expression 'y < 0' is always false. Unsigned type value is always >= 0. CountryListBox CountryLookup.cs 210
byte type values are in the [0; 255] range. Hence, the y < 0 check will always give false, and then branch will never be executed.
Issue 33
private void ParseTemplateInternal(...., string templatePath, ....)
{
....
string path = Path.Combine(templatePath, "admin.template");
if (!File.Exists(path))
{
// if the template is a merged copy of a localized templte the
// admin.template may be one director up
path = Path.Combine(templatePath, "..\admin.template");
}
....
}The PVS-Studio warning: V3057 The 'Combine' function is expected to receive a valid path string. Inspect the second argument. DotNetNuke.Library PortalController.cs 3538
Hmm. An interesting error. Here we have two operations to construct a path (the Path.Combine call). The first one is clear, but the second one is not. Apparently, in the second case, the developers wanted to take the admin.template file not from the templatePath directory, but from the parent one. Unfortunately, after they added ..\, the path became invalid since an escape sequence was formed: ..\admin.template.
Issue 34
internal override string GetMethodInformation(MethodItem method)
{
....
string param = string.Empty;
string[] names = method.Parameters;
StringBuilder sb = new StringBuilder();
if (names != null && names.GetUpperBound(0) > 0)
{
for (int i = 0; i <= names.GetUpperBound(0); i++)
{
sb.AppendFormat("{0}, ", names[i]);
}
}
if (sb.Length > 0)
{
sb.Remove(sb.Length - 2, 2);
param = sb.ToString();
}
....
}The PVS-Studio warning: V3057 The 'Remove' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. DotNetNuke.Log4Net StackTraceDetailPatternConverter.cs 67
Now, this code runs without any errors, but looking at it, I have a sneaking suspicion that something is wrong. In the then branch of the if statement, the value of sb.Length is >= 1. When the Remove method is called, we subtract 2 from this value. So, if sb.Length == 1, the call will be as follows: sb.Remove(-1, 2). This will cause an exception.
Right now, this code runs because, in StringBuilder, strings are added via the "{0}, " format. Therefore, these lines consist of 2 characters. A check like that is ambiguous and causes concerns.
Issues 35, 36
public void SaveJournalItem(JournalItem journalItem, int tabId, int moduleId)
{
....
journalItem.JournalId = this._dataService.Journal_Save(
journalItem.PortalId,
journalItem.UserId,
journalItem.ProfileId,
journalItem.SocialGroupId,
journalItem.JournalId,
journalItem.JournalTypeId,
journalItem.Title,
journalItem.Summary,
journalItem.Body,
journalData,
xml,
journalItem.ObjectKey,
journalItem.AccessKey,
journalItem.SecuritySet,
journalItem.CommentsDisabled,
journalItem.CommentsHidden);
....
}
public void UpdateJournalItem(JournalItem journalItem, int tabId, int moduleId)
{
....
journalItem.JournalId = this._dataService.Journal_Update(
journalItem.PortalId,
journalItem.UserId,
journalItem.ProfileId,
journalItem.SocialGroupId,
journalItem.JournalId,
journalItem.JournalTypeId,
journalItem.Title,
journalItem.Summary,
journalItem.Body,
journalData,
xml,
journalItem.ObjectKey,
journalItem.AccessKey,
journalItem.SecuritySet,
journalItem.CommentsDisabled,
journalItem.CommentsHidden);
....
}Here we have 2 issues. Looks as if they are multiplied by copy-paste. Try to find them! The answer is behind this picture.
Whoops, my bad! I forgot to give you a clue... Here you are:
int Journal_Update(int portalId,
int currentUserId,
int profileId,
int groupId,
int journalId,
int journalTypeId,
string title,
string summary,
string body,
string itemData,
string xml,
string objectKey,
Guid accessKey,
string securitySet,
bool commentsHidden,
bool commentsDisabled);Hope it's clear now. Found a problem? If not (or you don't want to do that), take a look at the analyzer warnings:
Notice the last parameters and arguments. In both calls, journalItem.CommentsDisabled comes before journalItem.CommentsHidden. However, the commentsHidden parameter comes before commentsDisabled. Yeah, that's suspicious.
Issue 37
private static DateTime LastPurge
{
get
{
var lastPurge = DateTime.Now;
if (File.Exists(CachePath + "_lastpurge"))
{
var fi = new FileInfo(CachePath + "_lastpurge");
lastPurge = fi.LastWriteTime;
}
else
{
File.WriteAllText(CachePath + "_lastpurge", string.Empty);
}
return lastPurge;
}
set
{
File.WriteAllText(CachePath + "_lastpurge", string.Empty);
}
}The PVS-Studio warning: V3077 The setter of 'LastPurge' property does not utilize its 'value' parameter. DotNetNuke.Library IPCount.cs 96
The fact that set-accessor does not use the value parameter is suspicious. So, it's possible to write something to this property, but the assigned value is... ignored. I found one place in the code, where the following property is assigned:
public static bool CheckIp(string ipAddress)
{
....
LastPurge = DateTime.Now;
....
}As a result, in this case, DateTime.Now will not be stored anywhere. Yes, we can say that DateTime.Now is indirectly recorded in the created file, but... Imagine what happens if we set another date instead of DateTime.Now — set accessor doesn't have any restrictions for that.
Issue 38
private void DisplayNewRows()
{
this.divTabName.Visible = this.optMode.SelectedIndex == 0;
this.divParentTab.Visible = this.optMode.SelectedIndex == 0;
this.divInsertPositionRow.Visible = this.optMode.SelectedIndex == 0;
this.divInsertPositionRow.Visible = this.optMode.SelectedIndex == 0;
}The PVS-Studio warning: V3008 The 'this.divInsertPositionRow.Visible' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 349, 348. DotNetNuke.Website Import.ascx.cs 349
Again, the variable is assigned twice – the whole expression is duplicated. Perhaps it's redundant. But maybe developers copied the expression and forgot to change it. Hmm... The last line effect?
Issue 39
public enum AddressType
{
IPv4 = 0,
IPv6 = 1,
}
private static void FilterRequest(object sender, EventArgs e)
{
....
switch (varArray[1])
{
case "IPv4":
varVal = NetworkUtils.GetAddress(varVal, AddressType.IPv4);
break;
case "IPv6":
varVal = NetworkUtils.GetAddress(varVal, AddressType.IPv4);
break;
}
....
}The PVS-Studio warning: V3139 Two or more case-branches perform the same actions. DotNetNuke.HttpModules RequestFilterModule.cs 81
I guess these case branches should not be identical. In the second case, AddressType.IPv6 should be used.
Issue 40
private static DateTime CalculateTime(int lapse, string measurement)
{
var nextTime = new DateTime();
switch (measurement)
{
case "s":
nextTime = DateTime.Now.AddSeconds(lapse);
break;
case "m":
nextTime = DateTime.Now.AddMinutes(lapse);
break;
case "h":
nextTime = DateTime.Now.AddHours(lapse);
break;
case "d":
nextTime = DateTime.Now.AddDays(lapse);
break;
case "w":
nextTime = DateTime.Now.AddDays(lapse);
break;
case "mo":
nextTime = DateTime.Now.AddMonths(lapse);
break;
case "y":
nextTime = DateTime.Now.AddYears(lapse);
break;
}
return nextTime;
}The PVS-Studio warning: V3139 Two or more case-branches perform the same actions. DotNetNuke.Tests.Core PropertyAccessTests.cs 118
Pay attention to "d" and "w" – the bodies of the case branches. They duplicate each other. Copy-paste... Copy-paste never changes. The DateTime type doesn't contain the AddWeeks method, however, the case branch "w" obviously must work with weeks.
Issue 41
private static int AddTabToTabDict(....)
{
....
if (customAliasUsedAndNotCurrent && settings.RedirectUnfriendly)
{
// add in the standard page, but it's a redirect to the customAlias
rewritePath = RedirectTokens.AddRedirectReasonToRewritePath(
rewritePath,
ActionType.Redirect301,
RedirectReason.Custom_Tab_Alias);
AddToTabDict(tabIndex,
dupCheck,
httpAlias,
tabPath,
rewritePath,
tab.TabID,
UrlEnums.TabKeyPreference.TabRedirected,
ref tabPathDepth,
settings.CheckForDuplicateUrls,
isDeleted);
}
else
{
if (customAliasUsedAndNotCurrent && settings.RedirectUnfriendly)
{
// add in the standard page, but it's a redirect to the customAlias
rewritePath = RedirectTokens.AddRedirectReasonToRewritePath(
rewritePath,
ActionType.Redirect301,
RedirectReason.Custom_Tab_Alias);
AddToTabDict(tabIndex,
dupCheck,
httpAlias,
tabPath,
rewritePath,
tab.TabID,
UrlEnums.TabKeyPreference.TabRedirected,
ref tabPathDepth,
settings.CheckForDuplicateUrls,
isDeleted);
}
else
....
}
....
}The PVS-Studio warning: V3030 Recurring check. The 'customAliasUsedAndNotCurrent && settings.RedirectUnfriendly' condition was already verified in line 1095. DotNetNuke.Library TabIndexController.cs 1097
The analyzer detects the following pattern:
if (a && b)
....
else
{
if (a && b)
....
}In this code fragment, the second condition will be false – the variables have not changed between calls.
However, here we hit the big jackpot! Besides the conditions, the blocks of code are duplicated. if with its then branch was entirely copied.
Issue 42
private IEnumerable<TabDto> GetDescendantsForTabs(
IEnumerable<int> tabIds,
IEnumerable<TabDto> tabs,
int selectedTabId,
int portalId,
string cultureCode,
bool isMultiLanguage)
{
var enumerable = tabIds as int[] ?? tabIds.ToArray();
if (tabs == null || tabIds == null || !enumerable.Any())
{
return tabs;
}
....
}The PVS-Studio warning: V3095 The 'tabIds' object was used before it was verified against null. Check lines: 356, 357. Dnn.PersonaBar.Library TabsController.cs 356
We've discussed a similar case before, but I decided to do this again and analyze it in more detail.
The tabIds parameter is expected to have a null value. Otherwise, why do we check tabIds == null? But something is messed up here again...
Suppose tabIds is null, then:
Turns out that the check failed.
Issue 43
And now take a chance to find an error yourself! I simplified the task for you. Below is a shortened method, I cut almost everything unnecessary. The original method contained 500 lines – doubt that you would find the error. Although, if you want, take a look at it – here's a link on GitHub.
If you figure out what's wrong, you'll definitely get endorphins rush. :)
private void SaveModuleSettings()
{
....
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.SKIN}",
this.ddlSkin.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.CODEMIRRORTHEME}",
this.CodeMirrorTheme.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.BROWSER}",
this.ddlBrowser.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.IMAGEBUTTON}",
this.ddlImageButton.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.FILELISTVIEWMODE}",
this.FileListViewMode.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.DEFAULTLINKMODE}",
this.DefaultLinkMode.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.USEANCHORSELECTOR}",
this.UseAnchorSelector.Checked.ToString());
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.SHOWPAGELINKSTABFIRST}",
this.ShowPageLinksTabFirst.Checked.ToString());
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.OVERRIDEFILEONUPLOAD}",
this.OverrideFileOnUpload.Checked.ToString());
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.SUBDIRS}",
this.cbBrowserDirs.Checked.ToString());
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.BROWSERROOTDIRID}",
this.BrowserRootDir.SelectedValue);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.UPLOADDIRID}",
this.UploadDir.SelectedValue);
if (Utility.IsNumeric(this.FileListPageSize.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.FILELISTPAGESIZE}",
this.FileListPageSize.Text);
}
if (Utility.IsNumeric(this.txtResizeWidth.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.RESIZEWIDTH}",
this.txtResizeWidth.Text);
}
if (Utility.IsNumeric(this.txtResizeHeight.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.RESIZEHEIGHT}",
this.txtResizeHeight.Text);
}
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.INJECTJS}",
this.InjectSyntaxJs.Checked.ToString());
if (Utility.IsUnit(this.txtWidth.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.WIDTH}",
this.txtWidth.Text);
}
if (Utility.IsUnit(this.txtHeight.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.HEIGHT}",
this.txtWidth.Text);
}
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.BLANKTEXT}",
this.txtBlanktext.Text);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.CSS}",
this.CssUrl.Url);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.TEMPLATEFILES}",
this.TemplUrl.Url);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.CUSTOMJSFILE}",
this.CustomJsFile.Url);
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.CONFIG}",
this.ConfigUrl.Url);
....
}Here's a picture to hide the answer. You'll find it right behind the unicorn.
Now, it's time to check yourself!
The PVS-Studio warning: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'txtHeight' variable should be used instead of 'txtWidth' DNNConnect.CKEditorProvider CKEditorOptions.ascx.cs 2477
Wow, the analyzer is very attentive! Here's the shortened code.
private void SaveModuleSettings()
{
....
if (Utility.IsUnit(this.txtWidth.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.WIDTH}",
this.txtWidth.Text); // <=
}
if (Utility.IsUnit(this.txtHeight.Text))
{
moduleController.UpdateModuleSetting(this.ModuleId,
$"{key}{SettingConstants.HEIGHT}",
this.txtWidth.Text); // <=
}
....
}Note that in the second case, we process 'height' variables, not 'width'. However, when we call the UpdateModuleSetting method, this.txtWidth.Text is passed instead of this.txtHeight.Text.
Issue N
Of course, these are not all warnings the analyzer found. I tried to select the most interesting and concise. The analyzer also issued interprocedural warnings and lots of others similar to those that we discussed. I guess the project developers are interested in warnings more than the readers.
Also, the analyzer issued false positives. We discussed some of them. I guess the analyzer developers are interested in other false positives more than the readers. So, I didn't write about them all.
In my opinion, the issues are diverse. You may say: "I would never make such errors!" But humans tend to make mistakes – this is totally normal! There are lots of reasons for this. That's why we regularly find new errors.
We also make mistakes. And sometimes false positives happen – we admit those issues and fix them. :)
As for code quality, is it enough to have a team of experts? I don't think so. You have to adopt a complex approach and use various tools/techniques to control code and product quality.
Let's sum it up:
P.S. By the way, what's your Top 10 warnings from this article? ;)