Thursday, September 3, 2009

Sufferings with switch smell

Introduction
In my first blog, Using enum in C# for smart coding, described code snippets suffer with switch smell. So, here, I am explaining what the switch smell is, what’s wrong with it and how to avoid it?
 
Most of the time, our source code suffers with several bad smells. Authors of a great book, Refactoring: Improving the Design of Existing Code, explain these smells with refactoring techniques. Here, I try to explain how switch smell introduces unnecessary complexity, reduces flexibility and leaves a class with vague responsibilities (from caller’s point of view).

Code Explanation
In the following two samples (Sample 1 & Sample 2), I have written a class, Calculator which can perform four basic arithmetic operation addition, subtraction, division and multiplication on two numbers.
Note: Here all code snippets are written in C#.

Sample 1:
public enum Operation
{
    Add,
    Subtract,
    Multiply,
    Divide
}
public class Calculator
{
    public double Calculate(double firstNo, double secondNo, Operation operation)
    {
        switch (operation)
        {
            case  Operation.Add:
            {
                return (firstNo + secondNo);
                break;
            }
            case Operation.Subtract:
            {
                return (firstNo - secondNo);
                break;
            }
            case Operation.Multiply:
            {
                return (firstNo * secondNo);
                break;
            }
            case Operation.Divide:
            {
                return (firstNo / secondNo);
                break;
            }
            default:
            {
                throw new ArgumentException("Incorrect Argument");
            }
        }
    }

Sample 2:
public class Calculator
{
    public double Add(double firstNo, double secondNo)
    {
        return (firstNo + secondNo);
    }
    public double Subtract(double firstNo, double secondNo)
    {
        return (firstNo - secondNo);
    }
    public double Multiply(double firstNo, double secondNo)
    {
        return (firstNo * secondNo);
    }
    public double Divide(double firstNo, double secondNo)
    {
        return (firstNo / secondNo);
    }
}

Calculator of Sample 1 provides these functionalities with its Calculate() method whereas Calculator of Sample 2 has four methods for four specific functionalities.
In Sample 1, I introduce switch statement inside Calculate() method to distinguish the request of client code whether it (client request) wants to add, subtract, multiply or divide. On the other hand, in Sample 2, as each functionality is implemented in separate method I haven’t bothered with condition-checking (switch statement).

Comparison
Now, the question is: Which class is better than other one?
From design point of view Calculator is responsible for four distinct arithmetic operations, so it should have four methods in implementation level.
In Sample 1, Calculator has only a single method, Calculate() which perform four arithmetic operations. As a result it represents poorly self-documentation and introduces unnecessary complexity. It suffers with switch smell. As a result Calculator responsibilities is not concrete here. So, caller of the Calculate() method has to decide what it (caller) wants from Calculate() method by setting the parameter.
On the other hand, in Sample 2, interface of Calculator is clearer and also it is self-documented.
Sample 2 is more flexible than Sample 1. Suppose if I want to provide another functionality to add three numbers.
For Sample 2, I quickly write overload Add() method as follows with reusability.


public double Add(double firstNo, double secondNo, double thirdNo)
{
    return Add(Add(firstNo, secondNo), thirdNo);
}

But what’s for Sample 1?  :-((. I have to follow some ugly and stupid ways to implement this functionality which introduces more complexity, code repetition, procedural thoughts etc.

More..
In Refactoring: Improving the Design of Existing Code, Authors explain this problem (Sample 1)  under bad small of switch statement and also show several ways (‘Replace type code with Subclasses’, ‘Replace type code with State/Strategy’, ‘Replace Parameter with Explicit Method’ how to refactor this smell. At the same time, authors describe ‘Parameterize Method’ which seems reverse of our discussion.

Conclusion
Each method of a class should represents a single activity, must be concrete and self-documented. One more activities in a single method (by switch smell) or one activity in several methods (I will discuss it another day) introduces complexicity and ambiguous interface, hinders changes, intencifies duplication and leave the class poorly self-documented.

Tuesday, August 18, 2009

Using enum in C# for smart coding




Most of us have written these (Sample 1 & Sample 2) kind of code snippet in our student or even professional life:

Sample 1 (C# Code):
string employeeType = employeeTypeComboBox.Text;
if (employeeType == "Permanent")
{
CalculateSalary();
}
else if (employeeType == "Worker")
{
CalculateWages();
}
else
{
// Do nothing
}

Sample 2 (C# Code):
string studentType = studentTypeComboBox.Text;
if (studentType == "Regular")
{
// Do Something
}
else if (studentType == "Private")
{
// Do Something
}
else
{
// Do nothing
}
In these two samples, user input is taken in a string valriable (employeeType /studentType) from a ComboBox and compare the variable with some discrete hard-coded string values (Permanent/Worker/Regular/Private).
What will happen if you type (some of you has faced these sad experiences) Parmanent instead of Permanent in Sample 1? Definitely, CalculateSalary() method will be not called and even program doesn’t show you any error message (any compile error). In best case, You will find out this problem during development and fix it immediately. But in worst case this problem arises after demployment.


Is there any solution where my typo will be detected earliar and show me a compile error?
Yes. Try emun.


See Sample Code 3, the more smart version of Sample code 1. Here you have no typo option.


Sample Code 3
enum EmployeeType
{
Permanent,
Worker
}
string employeeType = employeeTypeComboBox.Text;
if (employeeType == EmployeeType.Permanent.ToString())
{
CalculateSalary();
}
else if (employeeType == EmployeeType.Worker.ToString())
{
CalculateWages();
}
else
{
//Do Nothing
}

Believe me still you have chance to improve the code quality of Sample Code 3. Just see the Sample Code 4, I have wipe out all kinds of string variable from Sample Code 3.


Sample Code 4:
EmployeeType selectedEmployeeType = (EmployeeType)Enum.Parse(typeof(EmployeeType), employeeTypeComboBox.Text);
if (selectedEmployeeType == EmployeeType.Permanent)
{
CalculateSalary();
}
else if (selectedEmployeeType == EmployeeType.Worker)
{
CalculateWages();
}
else
{
//Do Nothing
}

Even, as your input comes from employeeTypeComboBox, don’t write hard-coded string in it’s item list, rather do it:
employeeTypeComboBox.DataSource = Enum.GetNames(typeof(EmployeeType));

So, why enum?
a) To improve code clarity
b) Make the code easier to maintain
c) Getting error at earlier stage

Note:
a) You can define enum inside or outside of class, but not inside of method or property.
b) The list of names contained by a particutar type of enum called enumerator list.


More About enum:
a) If you want you can keep value with each name in enumerator list. Example:


enum Priority
{
   Critical = 1,
   Important = 2,
  Medium = 3,
   Low = 4
};


You will get this value by type casting to the related type.
int priorityValue = (int) Priority.Medium;


b) Another nice thing, you can easily iterate through enumerator list.
foreach(Priority priority in Enum.GetValues(typeof(Priority)))
{
   MessageBox.Show(priority.ToString());
}
or even
for (Priority prio = Priority.Low; prio >= Priority.Critical; prio--)
{
   MessageBox.Show(prio.ToString());
}


Conclusion:
You should avoid string operation (specially comparison) because it decreases readability and introduces opacity. But using enum you can increase code readability, discover bugs in design time and keep your code more easy to maintain. So, Instead of string operation we should use enum (where applicable)  as described in this discussion.