Make your code better and faster

Some best practice solution to help to improve your code quality and - in given case - make it faster as well. A lot of small things can make your life easier, help to maintain your code and prevent possible bugs. So, lets jump in:

.Any() or .Length instead of Count()

In case of IEnumerable the content will be evaluated lazy, so try do it as late as possible. If you need to check whether an IEnumerable is empty or not always use .Any() == true instead of .Count() > 0. Why? For .Any() it is enough to find the first item, but for .Count() you need to go through the whole list. In case of high cardinal it can take a while. In the following two code snippets hasValue has the same result, both in the first case much faster. If it is not IEnumerable but an Array[] then .Length > 0 is even more faster then .Any() since it does not start any iteration.

// stops after the first item
var hasValue = myEnumerable.Any();

// iterate through
var hasValue = myEnumaerable.Count() > 0;

Access to Modified Closure

It is more like a bug than a best practice. There is no other way to do it. If you use ReSharper you probably have seen this warning before. Working with delegates in an iteration can be dangerous. Store them in a local variable before using them.

Typical mistake:

foreach (var tab in this.TabItems)
{
  tab.CustomCommand = new RelayCommand(() => 
      { 
        Debug.WriteLine(tab.Header);       
      });     
}

Because it will be evaluated later, when CustomCommand executes, every tab will produce the same output. How to fix it? Store tab.Header in a local variable.

foreach (var tab in this.TabItems)
{
  var header = tab.Header;
  tab.CustomCommand = new RelayCommand(() => 
      { 
        Debug.WriteLine(header);       
      });     
}

Event handling

Similar to the point mentioned above. Between event was fired and subscription was made can a lot of thing happening. Save the handler before use it.

var handler = this.CustomEventHandler;
if (handler != null)
{
  handler(this, EventArgs.Empty);
}

One exit point

This is more like a religion. My personal opinion is: one method one return. Means, do not return a result in the middle of the method. Especially do not break a void method. Makes extreme difficult to follow the code or to debug, furthermore the maintenance is also more difficult.

Try to avoid:

public void MyMethod()
{
  // do not use, negate the if statement instead
  if (someBooleanField == false)
  {
    return;
  }

  switch (someEnumField)
  {
    // don't
    case enumValue1: 
         return;
    case enumValue2:
         i+= 5;
         break;
  }
}

In case of logging or debugging it is much easier to add one line before the return statement instead of changing every return path. Something like this:

public string MyMethod(bool param)
{
  var result = string.Empty;
  
  if (param != false)
  {
    switch (this.MyEnum)
    {
      case EnumMember1: 
         result = "result string";
         break;
      case EnumMember2:
         result = "second";
         break;
    }
  }

  return result;
}

 

Do not use the exclamation mark (!)

Looks cool, make code shorter, but really easy to misread. Not a big deal to write == false and you saved yourself some headache next morning after checking in.

// you can read it, but can you do it at 9 AM, before your coffee? :)
if (!this.MyProperty && !(this.MyBoolean || this.MyBoolean2))
{ }

// a little bit better
if (this.MyProperty == false && (this.MyBoolean || this.MyBoolean2) == false)
{ }

And in the first case one "delete" or "backspace" can change the entire functionality without a compile error, in the second case, it is not gonna happening.

Outsourcing and the power of base classes

If your method is more than 30-50 lines, than you either dealing with some complex logic or you forgot to outsource. When you have more methods you can easily reuse them instead of (re)implementing it. Implementing a function twice (not in the same class, but in the whole solution!!!!) should cause some reconsideration: "is it really good enough?". But, when it comes to the third time, you definitely need to change your code.

Put common codes in base (abstract) classes, don't afraid to use them, they help to keep your code organized and help to avoid multiple implementations, too.

Evaluation order

Like I mentioned before, try to evaluate only when it's really needed. When you need to do some if-else statement put the fastens condition first and the slowest and the end. C# evaluates conditions in the given order and stop if the value is determined. So

if (this.MyBool == false && this.MyEnumerable.Any() && this.Validate(this) == false)
{ }

When MyBool is true, C# breaks and jumps into the else (if there is any) and does not evaluate MyEnumerable nor Validate the complete view model. You can save a lot of execution time.

Document your code smart

There a lot of tool, addin or 3rd party solution for documenting VS code. They try to figure out the the functionality from the name of methods, properties etc. Actually, I don't think they would be useful at any point. You're gonna end up generating meaningless documentation for every line of code. Let's see an example:

/// <summary>
/// Gets a value indicating whether is read only.
/// </summary>
public bool IsReadOnly { get; private set; }

/// <summary>
/// The Result
/// </summary>
private CustomResult Result;

/// <summary>
/// The my method
/// </summary>
private void MyMethod()
{ }

Every method, property, field is documented. But total meaningless, like this MSDN documentation. I guess you've already knew IsReadOnly gets or sets something, but what is it? How to make it better in your code? Document only public properties, methods and interfaces. Private fields, properties are for internal use only. Add some comment if it's needed. Unfortunately there is no document inheritance in VS, but it's enough to document virtual members once, where you define them.

/// <summary>
/// Is the complete page read only
/// </summary>
public bool IsReadOnly { get; private set; }

private CustomResult Result;

// do some internal calculation before proceed
private void MyMethod()
{ }

Isn't it better and faster? Actually if your naming convention good enough you have done half of the documentation task, the code documents itself. Some guidelines can be found on the MSDN Naming Guidelines site.