David Boschmans' Weblog

Code Review with a focus on Performance: Part 1

One of the most quoted truths about successful application development is that bugs must be found and fixed early. In practice, however, many bugs are only discovered late in the development cycle, or worse, during deployment of the application. 
The reason for this is that the process of detecting, locating, diagnosing and fixing bugs is labor-intensive and tedious. Activities such as peer-code review and code walk-through demand a lot of time and concentration of the staff that is most critical: senior developers.
Another reason is that many developers use their own coding standards, reducing the readability of their code for other developers. Even if corporate coding standards exist, they can only be enforced by labor-intensive code reviews, which are often discontinued when the deadlines get close.

Based on some recent reviewing experiences I decided to list a number of code review rules. These rules, defined in DevPartner and/or FxCop, can be used by (less-experienced) developers as guidelines during development time. Using these rules as Source Code Analysis will - hopefully - free up a lot of development time and improve .NET code quality and maintainability.
DevPartner Code Review Rule Manager organizes its rules set in the following categories:

• Date Formatting Rules
• Design Time Properties
• Internationalization Rules
• Logic Rules
• Naming Guidelines
• Performance Rules
• Web Applications

The rule base is fully configurable. Rules specific to your organization can easily be added and enforced.  Development groups can implement and enforce coding standards that make their applications more maintainable throughout the life cycle.
Note that the following rules list is not exhaustive, and that some reflection on the .NET Framework API will always help!

In this "Code Review" post you can find the first part (out of three) on performance rules.

1. Method contains multiple string concatenation, consider using StringBuilder [C# / VB.NET]

Use the StringBuilder class to concatenate strings, rather than these techniques:

[C#]
   string str = "";
   string str = str + "text";

Using one of these methods causes new string objects to be created every time text is added to the string. This is because the string object only contains a fixed length string. Its length cannot be dynamically increased in order to add text to it. Instead, a new string object is created that receives both the original string object's text and the new text that will be added.

Using the StringBuilder class:

[C#]
using System.Text;
    . . .
    string str = "";
    StringBuilder sb = new StringBuilder();
    sb.Append(str1);
    sb.Append("text");
    str = sb.ToString();

Using the StringBuilder class promotes faster string concatenation. Furthermore, string concatenation in an application that is intended to be translated into more than one language is generally considered improper coding practice. It might cause misleading or incoherent wording when the translated, concatenated strings are displayed.


2. String equality check using = or == operator found [C# / VB.NET]

Using = (Visual Basic .NET) or == (Visual C# .NET) operators to compare strings is slower than using the .NET Framework String class's String.Equals method.
In addition, if you need to do case sensitive or insensitive comparisons the String class's String.Compare method is the preferred way to do so. The String.Compare method can compare two strings based on their alphabetical sort order as well, something that is important in developing applications for the international market.

[C#]
Change:  string1 == string2 to string1.Equals(string2)


3. Found a jagged array declaration [C#]

Creating a rectangular array only requires one heap allocation for the System.Array type. Creating a jagged array of reference types requires N+1 System.Array heap allocation where N is the number in the first square bracket. You take a small performance hit when you use a jagged array. However, jagged arrays may make better use of memory when compared to a rectangular array large enough to hold the jagged array.

[C#]
object [] foo = new object[5]; // one heap allocation
object [][] foo = new object[2][]; // three heap allocations 
                                   // 1 for the main array and 2 more for the arrays referenced by the main array

If possible, and if the memory footprint is not a concern, convert a jagged array to a rectangular array.
Note that jagged arrays are not Common Language Specification (CLS)-compliant.


4. Reflecting errors from unmanaged code to managed code [C# / VB.NET]

When P/Invoke is used to call an unmanaged function and an exception is thrown, this exception is reflected back to unmanaged code only when the SetLastError property is set to true in the DLLImportAttribute attribute. However, setting this property to false improves performance. The default is false

[C#]
[ DllImport( "..\\LIB\\PinvokeLib.dll",
EntryPoint="?DoSomething@CTestClass@@QAEHH@Z",
CallingConvention=CallingConvention.ThisCall, SetLastError=true )]
public static extern int TestThisCalling( IntPtr ths, int i );   

Either:
Set SetLastError to true to retrieve any thrown errors from unmanaged code. Performance will be reduced.
Or:
Set SetLastError to false to increase performance. Any exceptions thrown in unmanaged code will be lost.


5. Excessive use of AutoPostBack attribute [HTML]

Excessive use of the AutoPostBack attribute on controls, such as the CheckBox and RadioButton, might result in decreased performance due to more frequent posts back to the server.

Change the logic of the program to examine the value of the controls on a submit.

6. Turn off tracing in Web pages [HTML]

Using the Trace option in an ASP Web Form can be very helpful for debugging. However, it is very costly in terms of performance.
In production, remove the Trace="true" attribute from the Page directive of the Web Form.

7. Turn off Debug attribute [HTML]

Enabling debugging on a Web form will lower the performance of the page.
Remove the Debug="true" attribute from the Page directive.


8. Excessive use of AutoPostBack attribute in Web User Control [HTML]

Excessive use of the AutoPostBack attribute on controls, such as the CheckBox and RadioButton, might result in decreased performance. This could be due to more frequent posts back to the server.

Change the logic of the program to examine the value of the controls on a submit.


9. Use of Preserve with Redim [VB.NET]

Use of Redim Preserve is costly in terms of performance to your application.
If possible, eliminate the use of Preserve with Redim by making your arrays large enough to hold all of the data.

[VB.NET]
ReDim Preserve MyArray(100)


10. Avoid building non-callable code into assemblies [C# / VB.NET]

A private or internal (assembly-level) member does not have callers in the assembly, is not invoked by the common language runtime, and the member is not invoked by a delegate. The following members are not checked by this rule:

• Explicit interface members
• Static constructors
• Static methods named 'Main' with no parameters or a single string array parameter
• Serialization constructors
• Methods marked with ComRegisterFunction or ComUnRegisterFunction
• Members that are overrides

Consider removing noncallable code, or add code that calls it.


11. Avoid building uninstantiated internal classes into assemblies [C# / VB.NET]

An instance of an assembly-level type is not created by code within the assembly. The following types are not examined by this rule:

• Value types
• Abstract types
• Enumerations
• Delegates
• Compiler-emitted array types
• Types that define static methods only

This rule attempts to locate a call to one of the type's constructors, and reports a violation if no call is found.
Consider removing the type, or add the code that uses it.


12. Properties should not return arrays [C# / VB.NET]

A public or protected property in a public type returns an array.
Arrays returned by properties are not write-protected, even if the property is read-only. To keep the array tamper-proof, the property must return a copy of the array. Typically, users will not understand the negative performance implications of calling such a property. Specifically, they might use the property as an indexed property. For additional information, see Properties vs. Methods in the Property Usage Guidelines.

To fix a violation of this rule, make the property a method.

The following example shows a property that violates this rule. The Main method illustrates how a user might write poorly performing code using such a property.

[C#]
using System;

namespace PerformanceLib
{
    public class Test
    {
        string [] nameValues;   

        public Test()
        {
            nameValues = new string[100];
            // Loading string array with sample data.
            for (int i = 0; i< 100; i++) 
            {
               nameValues[i] = "FxCop";
            }
     }

// Violates FxCop rule: PropertiesShouldNotReturnArrays.
        public string [] Names
        {
          get
          {
                return (string[]) nameValues.Clone();
          }
        }

        public static void Main()
        {
            // Using the property in the following manner
            // results in 201 copies of the array.
            // One copy is made each time the loop executes,
            // and one copy is made each time the condition is tested.

            Test t = new Test();
            for (int i = 0; i < t.Names.Length ; i++)
            {
                if (t.Names[i] == ("SomeName"))
                {
                    // Perform some operation.
                }
            }
        }
    }
}