Refactoring Gotcha

Today I am going to write about recent refactoring gotcha I ran into. Hopefully this won't be a long post as I usually do. So let's move directly to the coding example. I had a original code like this. (Which was needed to be refactored considering most recent requirement)

// For the sake of this example, let's assume we have following enums declared in the Obj-C file

typedef NS_ENUM(NSInteger, Month) {
    MonthJanurary,
    MonthFebruary,
    MonthMarch,
    MonthApril,
    MonthMay
};

// And then we have a method which takes a enum of this type and prints the appropriate log statement as follows,

- (void)valuePrinter:(Month)month {
    switch (month) {
        case MonthJanurary:
        case MonthFebruary:
            NSLog(@"Printing 1");
            break;
        case MonthMarch:
        case MonthApril:
            NSLog(@"Printing 2");
            break;
        case MonthMay:
            NSLog(@"Printing 3");
            break;
        default:
            break;
    }
}

Now let's looks at the logic of this very simple codebase,

If month is January or February - Print "Printing 1"
If month is March or April - Print "Printing 2"
if month if May - Print "Printing 3"

Very simple things, nothing fancy.

Now, what happened was I was told to remove the case for MonthFebruary. I thought this is the cakewalk and proceeded to remove that case and then I was under impression that everything remains intact,

So this is the final code after my seemingly innocuous refactor,

- (void)valuePrinter:(Month)month {
    switch (month) {
        case MonthJanurary:
        case MonthMarch:
        case MonthApril:
            NSLog(@"Printing 2");
            break;
        case MonthMay:
            NSLog(@"Printing 3");
            break;
        default:
            break;
    }
}

However, after it was gone for testing I was told something was broken. The case for MonthJanuary was not giving the expected result. So I went back, found the commit and trying to uncover what might have gone wrong.

Afterall, this was all very little refactor with no big changes. After spending some time, I realized my mistake during refactor. As we can see from above example, NSLog(@"Printing 1"); was relevant to months of both January and February. But during refactor, I took the whole block of first case off which resulted in completely eliminating original logic for the case of MonthJanurary and got it reassigned to the logic for months of March and April.

In ideal case, what should've happened is, only February case should've been removed and logic should've stayed there since we are still using that for the current present month of January, so it will still be executed.

Instead, I removed the logic for first two cases and assigned it to the third case.

Here's the corrected code after refactor where February case is removed and existing logic remains unchanged.

- (void)valuePrinter:(Month)month {
    switch (month) {
        case MonthJanurary:        
            NSLog(@"Printing 1");
            break;
        case MonthMarch:
        case MonthApril:
            NSLog(@"Printing 2");
            break;
        case MonthMay:
            NSLog(@"Printing 3");
            break;
        default:
            break;
    }
}

This bug is easier to miss in the Objective-C, since cases are defined on separate lines so it's possible to assume that the logic is applicable only for the last case.

This, however is mitigated in Swift where multiple cases are written on the same line making it obvious that the underlying logic is applicable to all the described cases separated by comma. Here's the similar example in Swift,

func valuePrinter(month: Month) {
    switch month {
    case .jan, .feb:
        print("Printing 1")
    case .mar, .apr:
        print("Printing 2")
    case .may:
        print("Printing 3")
    default:
        print("nothing")
    }
}

So if I was asked to remove February case in Swift, there is high chance that I would've done the right thing just by removing the feb case instead of deleting the entire logic for printing "Printing 1".

Gotchas like this are sometimes difficult to catch in early stages. It's good that we learn from our mistakes and make things better. One other advantage is of the language too. Stricter and clearer the language is, more difficult it is to fall into such traps or at least catch them as soon as they're created