CODEWITHSUNDEEP
c#unit-testingtdd
Zack
Zack

Reputation: 608

Programming & Unit Testing Best Practice

I've come across a scenario that I'm not too sure how to approach.

We're trying to get our unit tests code coverage to 100%.

I've been trying to take a TDD approach to development, writing tests, making it pass, writing a failing test, adding more code to make it pass, etc.

While doing this I found that I wrote a class like this:

public enum IntervalType : int
{
    Shift = 1,
    PaidBreak = 2,
    UnpaidBreak = 3
}


public static class QuarterFactory
{
   public static IQuarter CreateQuarter(IntervalType intervalType)
   {
      switch (intervalType)
      {
           case IntervalType.Shift:
                return new ShiftQuarter();
           default:
                throw new ArgumentException(String.Format("Unable to create a Quarter based on an IntervalType of: {0}", intervalType));
       }
    }
 }

As coding progressed the factory expanded to this:

public static class QuarterFactory
{
    public static IQuarter CreateQuarter(IntervalType intervalType)
    {
        switch (intervalType)
        {
            case IntervalType.Shift:
                return new ShiftQuarter();
            case IntervalType.PaidBreak:
                return new PaidBreakQuarter();
            case IntervalType.UnpaidBreak:
                return new UnpaidBreakQuarter();
            default:
                throw new ArgumentException(String.Format("Unable to create a Quarter based on an IntervalType of: {0}", intervalType));
        }
    }
}

The first question I'm asking is:
Now that the factory fulfills the enum, do I remove the default exception for the sake of code coverage, or do I keep the exception in there as the default in the event that someone adds a new type to the enum and forgets to modify the factory?

The second question I'm asking is:
If I decided to remove the exception and default the type to be UnpaidBreakQuarter - does it make sense to default the IQuarter returned to UnpaidBreakQuarter, or would this raise the question of "why is the default the UnpaidBreakQuarter, is there a business rule for this?".

Regards,

James

Upvotes: 4

Views: 388

Answers (4)

KarlM
KarlM

Reputation: 1662

You know that this sort of switch/enum combo is generally a code smell right? http://c2.com/cgi/wiki?SwitchStatementsSmell

You could refactor to use polymorphism: that would keep your 100% coverage, allow easy addition of new cases, and not need a default because the compiler would prevent it.

The two most common refactorings in this case are "Replace Type Code with Class" and the more specific "Replace Type Code with Strategy". Also "Replace Conditional with Polymorphism" is of course worth a look. http://refactoring.com/catalog/replaceConditionalWithPolymorphism.html

Upvotes: 0

Oskar Kjellin
Oskar Kjellin

Reputation: 21900

You can still get 100% code coverage. Consider the following line that compiles just fine:

QuarterFactory.CreateQuarter((IntervalType)0);

Thus, this answers the second question as well. You can get really confusing results if that would return an UnpaidBreakQuarter.

So in short:

  1. Keep the default, unit test that it throws exception if passing something unexpected (like the line above)
  2. Don't do it. If someone did not specify that they wanted this, they wouldn't expect it to be returned

You can even get the exception to be raised by doing this:

QuarterFactory.CreateQuarter(0);

Upvotes: 5

marco-fiset
marco-fiset

Reputation: 1933

I think you should keep the default at the expense of your 100% code coverage, unless there really is a business rule for this and it has been documented. If you choose to keep it, when someone adds another value in the enum, an exception will be thrown as a "reminder" to add the case in the switch statement, which I bet is good.

Upvotes: 1

Dippi
Dippi

Reputation: 397

I think you should keep the default block, it's a good practice, especially for the case you already mentioned, that is if someone would modify the code in the future adding a new IntervalType.

The answer to the second question comes consequently :) Btw, using the default for a specified value because "I know that the only value left is that" is really horrible, the default value was intended exactly for exceptions or unexpected cases, or at most for the most general ones (i.e: the first not, the second not, ok, so ANY other case should be this value).

Upvotes: 2

Related Questions