There are few principle we here at Reactuate Software try to keep in mind when writing code. One is DRY, or “Do not Repeat Yourself”. Another is “Readability Counts”. So when I see code that looks like this, I cringe.
- (IBAction)dayButtonPressed:(id)sender { UIButton *b = (UIButton*)sender; theDay = b.tag; switch (b.tag) { case 2: monButton.selected = YES; tueButton.selected = NO; wedButton.selected = NO; thButton.selected = NO; friButton.selected = NO; satButton.selected = NO; sunButton.selected = NO; break; case 3: monButton.selected = NO; tueButton.selected = YES; wedButton.selected = NO; thButton.selected = NO; friButton.selected = NO; satButton.selected = NO; sunButton.selected = NO; break; case 4: monButton.selected = NO; tueButton.selected = NO; wedButton.selected = YES; thButton.selected = NO; friButton.selected = NO; satButton.selected = NO; sunButton.selected = NO; break; case 5: monButton.selected = NO; tueButton.selected = NO; wedButton.selected = NO; thButton.selected = YES; friButton.selected = NO; satButton.selected = NO; sunButton.selected = NO; break; case 6: monButton.selected = NO; tueButton.selected = NO; wedButton.selected = NO; thButton.selected = NO; friButton.selected = YES; satButton.selected = NO; sunButton.selected = NO; break; case 7: monButton.selected = NO; tueButton.selected = NO; wedButton.selected = NO; thButton.selected = NO; friButton.selected = NO; satButton.selected = YES; sunButton.selected = NO; break; case 1: monButton.selected = NO; tueButton.selected = NO; wedButton.selected = NO; thButton.selected = NO; friButton.selected = NO; satButton.selected = NO; sunButton.selected = YES; break; default: break; } }
This code is in an iOS app I inherited and it handles this control.
To me this violates both the concept of DRY and the concept of Readability. It is pretty obvious what the routine does and you can even guess how it works. But the devil is in the details. Yes you know it is turning off all the buttons but the day that is selected. What if one of those case statements was messed up and accidentally had two buttons set to true? The sheer number of repeated very similar values would make it a pain to find.
Plus its just ugly and inelegant.
Here’s the 30 second fixed version:
- (IBAction)dayButtonPressed:(id)sender { UIButton *b = (UIButton*)sender; theDay = b.tag; monButton.selected = NO; tueButton.selected = NO; wedButton.selected = NO; thButton.selected = NO; friButton.selected = NO; satButton.selected = NO; sunButton.selected = NO; switch (b.tag) { case 2: monButton.selected = YES; break; case 3: tueButton.selected = YES; break; case 4: wedButton.selected = YES; break; case 5: thButton.selected = YES; break; case 6: friButton.selected = YES; break; case 7: satButton.selected = YES; break; case 1: sunButton.selected = YES; break; default: break; } }
See how much cleaner that is. There are situation where setting all the controls one way and then immediately setting them to on might cause a flicker, but iOS isn’t one of them.
Since I’m in this code anyway….
Here’s an even more cleaned up version
/* -------------------------------------------------------------------------------------------- dayButtonPressed Description: Action method for the day of the week frequency control. -------------------------------------------------------------------------------------------- */ - (IBAction)dayButtonPressed:(id)sender { UIButton* button = (UIButton*)sender; theDay = button.tag; monButton.selected = NO; tueButton.selected = NO; wedButton.selected = NO; thButton.selected = NO; friButton.selected = NO; satButton.selected = NO; sunButton.selected = NO; switch (button.tag) { case SUNDAY: sunButton.selected = YES; break; case MONDAY: monButton.selected = YES; break; case TUESDAY: tueButton.selected = YES; break; case WEDNESDAY: wedButton.selected = YES; break; case THURSDAY: thButton.selected = YES; break; case FRIDAY: friButton.selected = YES; break; case SATURDAY: satButton.selected = YES; break; default: NSAssert(false, @"Day button Tag and unknown value %d", button.tag); break; } }
I had to define some constants but it is much more readable now. I also hate one letter variables, so the button variable got renamed as well.
Added an assert for something that should never happen, namely the value of the tag not being between 1 and 7. NSAsserts are only handled in non-release builds, so there isn’t a chance of it crashing the shipping app.
Also added is my “short” routine header for Objective-C code. It just tells what the method does . It doesn’t bother to tell what the parameters or return values are, because saying it is an “action method” tells an experienced iOS/MacOS programmer all that information.