A variety of errors in C# code by the example of CMS DotNetNuke: 40 questions about the quality
Tags: csharp, dotnet, opensource, codequality
Published: 2021-11-24 08:29:27

![0890_DNN/image1.png](https://import.viva64.com/docx/blog/0890_DNN/image1.png)

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

DotNetNuke is an open\-source content management system \(CMS\) written mainly in C\#\. The source code is available on [GitHub](https://github.com/dnnsoftware/Dnn.Platform)\. The project is part of [the \.NET Foundation](https://dotnetfoundation.org/)\.

![0890_DNN/image2.png](https://import.viva64.com/docx/blog/0890_DNN/image2.png)

The project has its [website](https://www.dnnsoftware.com/), [Twitter](https://twitter.com/dnn), [YouTube channel](https://www.youtube.com/c/dotnetnuke/videos)\. 

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](https://dnncommunity.org/) 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](https://github.com/dnnsoftware/Dnn.Platform) \(see a screenshot below\) shows that the developers use the [NDepend](https://www.ndepend.com/) static analyzer to monitor code quality\.

![0890_DNN/image3.png](https://import.viva64.com/docx/blog/0890_DNN/image3.png)

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](https://pvs-studio.com/en/blog/posts/) to read some\.

## About the check

To check the project, I used the source code from [GitHub](https://github.com/dnnsoftware/Dnn.Platform) 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](https://pvs-studio.com/en/pvs-studio/) 7\.15 to perform the analysis\. Want to try the analyzer on your project? Click [here](https://pvs-studio.com/en/pvs-studio/try-free/) to open the page with all the necessary steps\. Have any questions? Don't understand something? Feel free to [contact us](https://pvs-studio.com/en/about-feedback/?is_question_form_open=true)\.

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

## Best warnings

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](https://pvs-studio.com/en/docs/manual/6532/)\.

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

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/v3010/) The return value of function 'Replace' is required to be utilized\. DotNetNuke\.Library CBO\.cs 1038

Here we have several curious cases:

* the developers wanted to remove the *"Info"* substring from *tableName* but forgot that C\# strings are immutable\. *tableName* remains the same\. The replaced string is lost, since the result of the *Replace* method call is not stored anywhere;
* the *tableName* variable initialized with an empty string is declared in the code\. Right after, the developers check whether *tableName* is an empty string\.

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](https://pvs-studio.com/en/w/v3022/) Expression 'string\.IsNullOrEmpty\(tableName\)' is always true\. DotNetNuke\.Library CBO\.cs 1032

**Best warnings\. Issue 3**

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
private void ProcessRequest(....)
{
  ....
  if (!result.RewritePath.ToLowerInvariant().Contains("tabId="))
  ....
}
```

The PVS\-Studio warning: [V3122](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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:

* *\!info\.Name\.Equals\("FileBrowserWindowWidth"\)*
* *\!info\.Name\.Equals\("FileBrowserWindowHeight"\)*

Three duplicate checks within the same expression – not bad\. I guess that's a record\!

**Best warnings\. Issue 9**

```csharp
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](https://pvs-studio.com/en/w/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\.

```csharp
   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**

```csharp
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](https://pvs-studio.com/en/w/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\. :\)

## Other warnings

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:

```csharp
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](https://pvs-studio.com/en/w/v3004/) The 'then' statement is equivalent to the 'else' statement\. DotNetNuke\.Website Search\.ascx\.cs 432

**Issues 12, 13**

```csharp
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](https://pvs-studio.com/en/w/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:

```csharp
public string Description { get; set; }
```

Here's one more fragment that contains the variable assigned to itself:

```csharp
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](https://pvs-studio.com/en/w/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*\):

```csharp
/// <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\. :\)

```csharp
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](https://pvs-studio.com/en/w/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:

```csharp
public override void Install()
{
  ....
  skinFile.Replace(Globals.HostMapPath + "\\", "[G]");
  ....
}
```

The PVS\-Studio warning: [V3010](https://pvs-studio.com/en/w/v3010/) The return value of function 'Replace' is required to be utilized\. DotNetNuke\.Library SkinInstaller\.cs 230

**Issue 16**

```csharp
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](https://pvs-studio.com/en/w/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:

* if *this\.Page* is in the \[1; *int\.MaxValue*\] range, *pageIndex* will be in the \[0; *int\.MaxValue \- 1*\] range
* if *this\.Page* is in the \[*int\.MinValue*; 0\] range, *pageIndex* will have the 0 value\.

Therefore, the *pageIndex < 0* check will always be *false*\.

**Issue 17**

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
public JournalEntity(string entityXML)
{
  ....
  XmlDocument xDoc = new XmlDocument { XmlResolver = null };
  xDoc.LoadXml(entityXML);
  if (xDoc != null)
  ....
}
```

The PVS\-Studio warning: [V3022](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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:

* *result\.Action == ActionType\.Redirect302Now*
* *result\.Action == ActionType\.Redirect302*

Since *result\.Action* cannot have two different values, the described condition is impossible\. Therefore, the expression is always true\.

**Issue 20**

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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](https://pvs-studio.com/en/blog/terms/6461/)\.

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:

```csharp
// 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:

![0890_DNN/image4.png](https://import.viva64.com/docx/blog/0890_DNN/image4.png)

By the way, it turns out that:

* if *obj == null*, *stringRepresentation \!= null* \(an empty string\);
* if *obj \!= null*, *stringRepresentation == null*\.

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](https://pvs-studio.com/en/blog/posts/csharp/0656/) \(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:

```csharp
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\!

![0890_DNN/image5.png](https://import.viva64.com/docx/blog/0890_DNN/image5.png)

Grabbed a snack? We're proceeding to the next issue\.

**Issues 22, 23**

```csharp
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](https://pvs-studio.com/en/w/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\.

```csharp
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](https://pvs-studio.com/en/w/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:

```csharp
var settings = new Dictionary<string, object>
{
  ....
  { "portalName", 
    PortalSettings.Current.PortalName },                         
  ....
  { "currentPageName", 
    PortalSettings.Current?.ActiveTab?.TabName },           
  ....
  { "currentParentHasChildren", 
    PortalSettings.Current?.ActiveTab?.HasChildren },
  ....
};
```

**Issues 24, 25, 26**

```csharp
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](https://pvs-studio.com/en/w/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:

```csharp
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](https://pvs-studio.com/en/w/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\.\.\.

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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:

* V3095 The 'e\.FullPath' object was used before it was verified against null\. Check lines: 160, 165\. DotNetNuke\.Web DotNetNukeShutdownOverload\.cs 160
* V3095 The 'e\.FullPath' object was used before it was verified against null\. Check lines: 173, 178\. DotNetNuke\.Web DotNetNukeShutdownOverload\.cs 173
* V3095 The 'e\.FullPath' object was used before it was verified against null\. Check lines: 186, 191\. DotNetNuke\.Web DotNetNukeShutdownOverload\.cs 186

I think that's enough for [V3095](https://pvs-studio.com/en/w/v3095/)\. And I guess you don't want to read about it anymore\. So, let's move on\.

**Issue 31**

```csharp
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](https://pvs-studio.com/en/w/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:

```csharp
public const string FOLDER_ValidFolderPath = "C:\\folder";
```

**Issue 32**

```cpp
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](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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: *\.\.**\\a**dmin\.template*\.

**Issue 34**

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
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\.

![0890_DNN/image6.png](https://import.viva64.com/docx/blog/0890_DNN/image6.png)

Whoops, my bad\! I forgot to give you a clue\.\.\. Here you are:

```csharp
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:

* V3066 Possible incorrect order of arguments passed to 'Journal\_Save' method: 'journalItem\.CommentsDisabled' and 'journalItem\.CommentsHidden'\. DotNetNuke\.Library JournalControllerImpl\.cs 125
* V3066 Possible incorrect order of arguments passed to 'Journal\_Update' method: 'journalItem\.CommentsDisabled' and 'journalItem\.CommentsHidden'\. DotNetNuke\.Library JournalControllerImpl\.cs 253

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

```csharp
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](https://pvs-studio.com/en/w/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:

```csharp
public static bool CheckIp(string ipAddress)
{
  ....
  LastPurge = DateTime.Now;
  ....
}
```

As a result, in this case, *DateTime\.Now* will not be stored anywhere\.

**Issue 38**

```csharp
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](https://pvs-studio.com/en/w/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](https://pvs-studio.com/en/blog/posts/cpp/0260/)?

**Issue 39**

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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**

```csharp
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](https://pvs-studio.com/en/w/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:

```csharp
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**

```csharp
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](https://pvs-studio.com/en/w/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:

* the left operand of the ?? operator is evaluated \(*tabIds as int\[\]*\);
* *tabIds as int\[\]* results in *null*;
* the right operand of the ?? operator is evaluated \(*tabIds\.ToArray\(\)*\);
* the *ToArray* method call leads to an exception because *tabIds* is *null*\.

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](https://github.com/dnnsoftware/Dnn.Platform/blob/05544c719981842c341ec40583be2e149aeb64a9/DNN%20Platform/Providers/HtmlEditorProviders/DNNConnect.CKE/CKEditorOptions.ascx.cs)\.

If you figure out what's wrong, you'll definitely get endorphins rush\. :\)

```csharp
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\.

![0890_DNN/image7.png](https://import.viva64.com/docx/blog/0890_DNN/image7.png)

Now, it's time to check yourself\! 

The PVS\-Studio warning: [V3127](https://pvs-studio.com/en/w/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\.

```csharp
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\.

## Conclusion

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](https://pvs-studio.com/en/blog/inspections/)\.

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:

* be careful with copy\-paste;
* [use static analysis](https://pvs-studio.com/en/);
* follow [me on Twitter](https://twitter.com/_SergVasiliev_)\.

**P\.S\.** By the way, what's your Top 10 warnings from this article? ;\)