# A variety of errors in C\# code by the example of CMS DotNetNuke: 40 questions about the quality

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

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

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

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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/docs/warnings/v3022/) Expression 'string\.IsNullOrEmpty\(tableName\)' is always true\. DotNetNuke\.Library CBO\.cs 1032

**Best warnings\. Issue 3**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

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

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

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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\.

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

```cpp
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/docs/warnings/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:

```cpp
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/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. DotNetNuke\.Website Search\.ascx\.cs 432

**Issues 12, 13**

```cpp
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/docs/warnings/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:

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

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

```cpp
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/docs/warnings/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_\):

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

```cpp
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/docs/warnings/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:

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

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

**Issue 16**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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:

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

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

```cpp
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/docs/warnings/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\.

```cpp
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/docs/warnings/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:

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

**Issues 24, 25, 26**

```cpp
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/docs/warnings/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:

```cpp
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/docs/warnings/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\.\.\.

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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/docs/warnings/v3095/)\. And I guess you don't want to read about it anymore\. So, let's move on\.

**Issue 31**

```cpp
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/docs/warnings/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:

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

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

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

```cpp
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/docs/warnings/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:

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

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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**

```cpp
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/docs/warnings/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:

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

```cpp
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/docs/warnings/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\. :\)

```cpp
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/docs/warnings/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\.

```cpp
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? ;\)