Ashish Sheth's Blog

Frequently you will see following type of code in your code base.

if(result != 1)
{
    if(someOtherResult == 101)
    {
        if(anotherValue == 500)
        {
            // do something
        }
    }
    else
    {
        // do some other thing
    }
}

return;

Here the code forms a shape of an arrow-head, as below:

if
    if
        if
            if
                do something
            end
        end
    end
end

If you see, the main logic is deep down into the nested condition, it increases the cyclomatic complexity of the code.

A better version of the same code could be as below:

if(result == 1)
{
    return;
}

if(someOtherResult == 101 && anotherValue == 500)
{
    // do something
    return;
}

// do some other thing

return;

The above code does a number of things to flatten the code and make it better:

1. Validations are performed first and it returns at the first opportunity.
2. Multiple nested conditions are combine into one (with “&&”(logical And) operator. If there are multiple expressions forming one such condition, they can be moved to a separate method returning boolean. That method can then be use in the if condition as below:

if(IsValidResult(someOtherResult, anotherResult)
{
    // do something
    return;
}

bool IsValidResult(int someOtherResult, int anotherResult)
{
    if(someOtherResult == 101 && anotherValue == 500)
    {
        return true;
    }
    return false;
}

Tags: C#, design principles, AntiPatterns

Single Responsibility Principle (SRP)says that "THERE SHOULD NEVER BE MORE THAN ONE REASON FOR A CLASS TO CHANGE.". Although this article mentions only about class, I think the SRP also applies to methods within the class: there should never be more than one reason for a method to change.
Visual Studio provides a good way to mark off section of file in the form of "region"so they can be collapsible and the code can be organized. Many people use region in a big method to organize the code. For example:

public void CreeateOrder(/*some parameters*/)
{
	#region Validate the parameters
	//code goes here
	#endregion

	#region create the order
	//insert the order data in the database
	#endregion

	#region create the order item
	//insert the item data in the database
	#endregion
}

Note that not all people use regions like this. Many people use comments instead of regions in this kind of methods.
As you can see this is a clear violation of the single responsibility principle. The method does more than one thing: it validates the order data, create a top level order and create order items. This can certainly be put into separate method.

private bool ValidateOrderData(/*some parameters*/)
{
	#region Validate the parameters
	//code goes here
	#endregion
}

private bool InsertOrder(/*order related parameter*/)
{
	#region create the order
	//insert the order data in the database
	#endregion
}

private bool InsertOrderItem(/*order item related parameter*/)
{
	#region create the order item
	//insert the item data in the database
	#endregion
}

public  void CreateOrder(/*Some parameter*/)
{
	If(ValidateOrder(/*parameter list*/))
	{
		if(InsertOrder(/*order parameter*/))
		{
			InsertOrderItem(/*order item parameter*/);
		}	
	}
}

As you can see, wherever you are using "region" in your method to demarcate the code, you can very well put that code in a separate method.

Tags: vs2008, vs 2010, .net, c#, design principles