Click here to Skip to main content
Click here to Skip to main content

Code Smells - Switch and If Statements

, 30 Nov 2013
Rate this:
Please Sign up or sign in to vote.
Refactoring code smell

Introduction

Some time ago, I found an interesting site with bad code examples. Code from this site shows bad practice in code writing, but does not correct it in the right way. And I decided to do it with some examples from this portal.

Initial Source Code

The initial source code was got from http://govnokod.ru/14088.

public static int GetCarMaxRoomNumberNominal(string trainName, TrainCar car)
{
    if (!String.IsNullOrEmpty(trainName))
    {
        trainName = trainName.ToUpper();
        if (trainName.StartsWith("САПСАН"))
        {
            return 66;
        }
        else if (trainName.StartsWith("ЛАСТОЧКА"))
        {
            return 103;
        }
        else if (trainName.StartsWith("АЛЛЕГРО"))
        {
            return 72;
        }
    }
    
    switch (car.Category)
    {
        case TrainCarCategory.ReservedSeat:
        case TrainCarCategory.Common:
            return 54;
        case TrainCarCategory.Compartment:
            return car.TwoStorey ? 112 : 36;
        case TrainCarCategory.Lux:
        case TrainCarCategory.Soft:
            return car.TwoStorey ? 96 : 18;
        case TrainCarCategory.Sedentary:
            if (car.ServiceClass.Contains("1C"))
            {
                return 42;
            }
            else if (car.ServiceClass.Contains("2C"))
            {
                return 80;
            }
            if (car.ServiceClass.Contains("3C"))
            {
                return 117;
            }
            return 1;
        default:
            return 1;
    }
}

Let's Begin

First of all, I've added the needed classes and enumerations. After that, I wrote unit tests for this method. And only then, I started to refactor it.

This example of code smells shows us the usage of switch statement. Switch in object oriented code is canonical code smell.

Classical refactoring for its code is usage dictionary instead of switch.

I created the following table for analysing relationship between dictionary keys and values.

As you can see, this key is complex and has a variable number of parts.

I created the following classes for dictionary's key:

GenericKey.cs

public class GenericKey<TParts> where TParts : new()
{
    #region Properties
 
    public TParts Parts { get; set; }
 
    #endregion Properties
 
    #region Constructor
 
    public GenericKey()
    {
        Parts = new TParts();
    }
 
    public GenericKey(TParts inItems)
    {
        Parts = inItems;
    }
 
    #endregion Constructor
    
    #region Public methods
 
    public override bool Equals(object obj)
    {
        return obj != null && Equals((GenericKey<TParts>)obj);
    }
 
    public bool Equals(GenericKey<TParts> anotherKey)
    {
        return anotherKey != null && Parts.Equals(anotherKey.Parts);
    }
 
    public override int GetHashCode()
    {
        return Parts.GetHashCode();
    }
    
    #endregion Public methods
} 

KeyParts.cs

    public class KeyParts : IEnumerable
    {
        #region Fields
 
        private readonly List<KeyPart> _items;
        private int _position;
        private const int PutAllInTheSameBasket = 0;
 
        #endregion Fields
 
        #region Property
 
        public int Count
        {
            get
            {
                return _items.Count;
            }
        }
 
        public KeyPart Current
        {
            get
            {
                return _items[_position];
            }
        }
 
        #endregion Property
 
        #region Constructor
 
        public KeyParts()
        {
            _items = new List<KeyPart>();
        }
 
        #endregion Constructor
 
        #region Private methods
 
        private static bool Compare(KeyParts parts1, KeyParts parts2)
        {
            return parts1.Count == parts2.Count
                && (
                    from KeyPart key1Part in parts1
                    select parts2.Cast<KeyPart>().Any(
                        key2Part => key1Part.Name == key2Part.Name
                        && key1Part.Value == key2Part.Value)
                    ).All(part => part);
        }
 
        #endregion Private methods
 
        #region Public methods
 
        public KeyParts Add(KeyPartNames inName, object inValue)
        {
            return Add(new KeyPart(inName, inValue));
        }
 
        public KeyParts Add(KeyPart part)
        {
            if (part.IsValid)
            {
                _items.Add(part);
            }
            return this;
        }
        
        public override bool Equals(object obj)
        {
            return obj != null && Equals((KeyParts)obj);
        }
 
        public bool Equals(KeyParts anotherKey)
        {
            return anotherKey != null && Compare(this, anotherKey);
        }
 
        public override int GetHashCode()
        {
            return PutAllInTheSameBasket;
        }
 
        public IEnumerator GetEnumerator()
        {
            return _items.GetEnumerator();
        }
 
        public bool MoveNext()
        {
            _position++;
            return (_position < _items.Count());
        }
 
        public void Reset()
        {
            _position = 0;
        }
 
        #endregion Public methods
    } 

KeyPart.cs

public class KeyPart
{
    public KeyPartNames Name { get; private set; }
    public string Value { get; private set; }
    
    public bool IsValid
    {
        get { return !string.IsNullOrWhiteSpace(Value); }
    }
    
    private static string AsString(object value) 
    {
        if (value == null)
        {
            return string.Empty;
        }
        var enumValue = value as Enum;
        return enumValue != null ? enumValue.GetStringValue() : value.ToString();
    }
    
    public KeyPart(KeyPartNames inName, object inValue)
    {
        if (inValue != null && !(inValue is string) && 
        !(inValue is Enum) && !(inValue is bool))
        {
            throw new ArgumentException
            ("inValue may be only string, enum or bool");
        }
        Name = inName;
        Value = AsString(inValue);
    }
}

The initial source was refactored into this:

private static readonly Dictionary<GenericKey<KeyParts>, 
int> MaxRoomNumberNominals = new Dictionary<GenericKey<KeyParts>, int>
   {{ new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Sedentary}}}, 1},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.None}}},1},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Name, Name.Peregrin}}}, 66},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Name, Name.Swallow}}}, 103},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Name, Name.Allegro}}}, 72},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.ReservedSeat}}}, 54},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Common}}}, 54},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Compartment},
                                          {KeyPartNames.TwoStorey, true}}}, 112},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Compartment},
                                          {KeyPartNames.TwoStorey, false}}}, 36},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Lux},
                                          {KeyPartNames.TwoStorey, true}}}, 96},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Lux},
                                          {KeyPartNames.TwoStorey, false}}}, 18},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Soft},
                                          {KeyPartNames.TwoStorey, true}}}, 96},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Soft},
                                          {KeyPartNames.TwoStorey, false}}}, 18},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Sedentary},
                                          {KeyPartNames.Class, Service.First}}}, 42},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Sedentary},
                                          {KeyPartNames.Class, Service.Second}}}, 80},
    { new GenericKey<KeyParts> { Parts = {{KeyPartNames.Category, Category.Sedentary},
                                          {KeyPartNames.Class, Service.Third}}}, 117}};
                                          
/// <summary>
/// Source code after refactoring
/// </summary>
/// <param name="trainName"></param>
/// <param name="car"></param>
/// <returns></returns>
public static int GetCarMaxRoomNumberNominal(string trainName, TrainCar car)
{
    if (car == null) throw new ArgumentNullException("car");
    var key = RoomNumberKeyFactory.Create(trainName, car);
    return MaxRoomNumberNominals[key];
} 

History

  • November 30, 2013 - Initial release

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Share

About the Author

Dmitry Handin
Software Developer
Russian Federation Russian Federation
No Biography provided
Follow on   LinkedIn

Comments and Discussions

 
GeneralMy vote of 1 Pinmembertyaramis1-Dec-13 20:40 
GeneralRe: My vote of 1 PinprofessionalDmitry Handin1-Dec-13 20:48 
GeneralRe: My vote of 1 Pinmemberzaphoed3-Dec-13 5:47 
GeneralRe: My vote of 1 PinprofessionalDmitry Handin3-Dec-13 6:38 
GeneralRe: My vote of 1 Pinmemberzaphoed3-Dec-13 7:31 
QuestionDifferent solution Pinmemberervegter1-Dec-13 9:26 
AnswerRe: Different solution PinmemberDmitry Handin1-Dec-13 9:54 
GeneralRe: Different solution Pinmemberervegter1-Dec-13 10:59 
GeneralMy vote of 1 PinprofessionalRon Beyer30-Nov-13 12:26 
GeneralMy vote of 1 PinprofessionalPaulo Zemek30-Nov-13 11:11 
GeneralMy vote of 1 Pinmemberabdurahman ibn hattab30-Nov-13 9:52 
GeneralMy vote of 5 PinmemberVolynsky Alex30-Nov-13 6:47 
GeneralRe: My vote of 5 PinprofessionalDmitry Handin1-Dec-13 21:22 
GeneralRe: My vote of 5 PinmemberVolynsky Alex2-Dec-13 11:31 
SuggestionThoughts for improvements PinmemberCHill6030-Nov-13 5:23 
GeneralRe: Thoughts for improvements PinprofessionalDmitry Handin1-Dec-13 21:35 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.

| Advertise | Privacy | Mobile
Web01 | 2.8.140814.1 | Last Updated 30 Nov 2013
Article Copyright 2013 by Dmitry Handin
Everything else Copyright © CodeProject, 1999-2014
Terms of Service
Layout: fixed | fluid