Peter's Software House

Magic Numbers – How to remove them from your Codebase.

I am not a fan Magic Numbers at all.  But most code bases I have seen and/or worked on have them too varying degrees.  Using simple OO techniques can help you avoid them.  In this post I am going to discuss how to avoid Magic Numbers. 

Let’s start by creating a simple class of say a Person.

public class Person
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public char Status { get; set; }
}

Nothing special, just a firstname, lastname and status.  Let’s write a test to use it (Yea I know, should have written the test first!)

[TestMethod]
public void TestMethod1()
{
    Person p = new Person();
    p.Status = 'C';
    Assert.AreEqual(p.Status, 'C');
}

Ok, this works, but WTF does ‘C’ mean?  Closed? Created? (The test Name doesn’t help)

Let’s make a change to the Test to make it clear:

[TestMethod]
public void Person_Status_Closed()
{
    Person p = new Person();
    p.Status = Person.Closed;
    Assert.AreEqual(p.Status, Person.Closed);
}

So we changed the ‘C’ to a constant called Closed, which we have added to the Person class. This is cool for now, when I read this code it is clear that the status I am assigning to the Person class is Closed. A funny status to pick when you have just created the object, so let’s say the default status for a person is Active. Let’s write a test for that first:

[TestMethod]
public void PersonStatus_Default_Active()
{
    Person p = new Person();
    Assert.AreEqual(p.Status, Person.Active);
}

And let’s update the person class:

public class Person
{
    public const char Active = 'A';
    public const char Closed = 'C';

    public string FirstName { get; set; }
    public string LastName { get; set; }
    public char Status { get; set; }

    public Person()
    {
        Status = Active;
    }
}

All going pretty well so far.  But this does not stop us from using Magic Numbers, i.e. I can still check for ‘A’ or ‘C’ or I can assign any char value I like to status.  This is especially likely to happen when you are working on a large codebase and an inexperienced developer has to make a change in code and their first response is to use ‘A’ instead of Person.Active.  Sure this should be picked up during code reviews, but why not make it impossible to do this in the first place?  How?  Very easily, replace the Person.Status with a Status class.

Let’s create the test first:

[TestMethod]
public void PersonStatus_Default_Active()
{
    Person p = new Person();
    Assert.AreEqual(p.Status, PersonStatus.Active);
}

A simple change, can you spot it?

Let’s have a look at the implementation:

public class Person
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public PersonStatus Status { get; set; }

    public Person()
    {
        Status = PersonStatus.Active;
    }
}
public class PersonStatus
{
    public static PersonStatus Active = new PersonStatus('A', "Active");
    public static PersonStatus Closed = new PersonStatus('C', "Closed");

    public char Code { get; set; }
    public string Description { get; set; }

    public PersonStatus(char code, string description)
    {
        Code = code;
        Description = description;
    }
}

A huge improvement, you now have to use a PersonStatus object to indicate the status of Person.  But you can still create your own version PersonStatus, i.e. you can set status = new PersonStatus(‘X’, ‘Invalid Status’) and it will work.  Let’s add some validation. 

Test first:

[TestMethod]
[ExpectedException(typeof(InvalidStatusException))]
public void PersonStatus_InvalidStatus_ExpectException()
{
    Person p = new Person();
    p.Status = new PersonStatus('X', "Invalid Status");
    Assert.Fail("Expected an InvalidStatusException");
}

And the Implementation:

public class Person
{
    public string FirstName { get; set; }
    public string LastName { get; set; }

    private PersonStatus _status = PersonStatus.Active;
    public PersonStatus Status 
    {
        get
        {
            return _status;
        }
        set
        {
            if (!PersonStatus.IsValid(value))
                throw new InvalidStatusException(string.Format("'{0}' - {1} is an invalid status", value.Code, value.Description));
            _status = value;
        }
    }
}
public class PersonStatus
{
    public static PersonStatus Active = new PersonStatus('A', "Active");
    public static PersonStatus Closed = new PersonStatus('C', "Closed");

    private static IList<PersonStatus> _validStatusList = new List<PersonStatus>();

    public char Code { get; set; }
    public string Description { get; set; }

    static PersonStatus()
    {
        _validStatusList.Add(Active);
        _validStatusList.Add(Closed);
    }

    public static bool IsValid(PersonStatus status)
    {
        return _validStatusList.Contains(status);
    }

    public PersonStatus(char code, string description)
    {
        Code = code;
        Description = description;
    }
}
[Serializable]
public class InvalidStatusException : Exception
{
    public InvalidStatusException() { }
    public InvalidStatusException(string message) : base(message) { }
    public InvalidStatusException(string message, Exception inner) : base(message, inner) { }
    protected InvalidStatusException(SerializationInfo info, StreamingContext context)
        : base(info, context) { }
}