views:

162

answers:

3

I just finished reading the "Functions" chapter from Uncle Bob's Clean Code. The main advice was to make sure that functions are short -- really short. And they should only do one thing per level of abstraction. Here's a function from an app I'm making to learn Cocoa (idea from Andy Matuschak).

- (IBAction)go:(id)sender
{
NSString *output = nil;

if ([[nameInputField stringValue] isEqualToString:@""])
{
    output = @"Please enter your name";
}
else
{
    NSString *date = [[NSDate date] descriptionWithCalendarFormat:@"%A, %B %d" 
                                                         timeZone:nil 
                                                           locale:[[NSUserDefaults standardUserDefaults] dictionaryRepresentation]];
    output = [NSString stringWithFormat:@"Hello, %@! Today is %@.", [nameInputField stringValue], date];
}

[responseOutputField setStringValue:output];
}

Basically, this function reads a name from a text field (nameInputField) and outputs a message to another text field (responseOutputField) I'm wondering a) if this function does 'one thing' per level of abstraction and b) how to make it shorter.

+1  A: 

It does two things. First it gets/determines the output to print. Then it prints it. You could separate these. But I wouldn't. That seems like going too far to me.

Fletcher Moore
+1, agree. Drinking all of the KoolAid is almost always a bad idea, whether its programming advice or Jonestown. Rule #1 should always be 'use your brain'
StingyJack
+2  A: 

I think this function does a reasonable amount of work, and does not need to be broken down further.

I would suggest changing the name of the function to more clearly describe what it does (i.e. updateResponse). This will make the code easier to understand, when looking at the source and when looking at the NIB in interface builder. Also, if you can't find a name that succinctly describes what the function does, its a tip that you are violating the "one thing" goal.

You also ask how to make this code shorter. I think in this case you might be able to use bindings to keep the responseOutputField in sync with the nameInputField, without any code at all (depending on how exactly you want things to behave).

vkit
+1 Naming functions properly is extremely important IMO, and if I had to choose between a really short function and a well named one, I think I'd go with the well named one.
Fletcher Moore
You could use Bindings to, for example, disable the button that sends the `updateResponse:` message until the user has entered a name. Furthermore, you could have Key-Value Validation on the bound-to `name` property to ensure that the user enters at least one letter, and to strip off leading and trailing whitespace.
Peter Hosey
+2  A: 

I disagree that this function is at the right level. The core computation of working out what to output based on current input should be factored into another function. This will make that computation much more testable (since you don't need any text fields, you can unit test in isolation) and reusable, since it has much less contextual baggage. As it is, the function is hard-wired to a specific use and so is not reusable.

As it is, how do you test it without actually running the application?

mdma