If-Methods Redux

If-Methods Redux 27

Posted by Michael Feathers Fri, 26 Sep 2008 17:05:00 GMT

So was looking over my blog on If-Methods this morning and I realized that I fell into a trap. I was solving one problem but thinking about another.

There I was working on lexer code and I thought “Hmm… I’d really like to extract these if-blocks.” I mulled it over a bit, and while thinking about the general problem of naming if-blocks, I settled on a naming scheme that I thought would be good in general, but it ended up being a bit odd in particular. The comments on the blog show this. Many of them offer good suggestions for dealing with the particular case I showed, but I was really after something different, a good general-purpose naming scheme for methods extracted around conditional.

I’m going to try this again and see if I can come up with a case which motivates a generic naming scheme a bit better.

Consider this chunk of code.


    ...
    if (sensorTripped) {
        ...
        alarm.sound();
    }
    ...

We could extract the body of the if-block, but in a long method, we might want to extract the entire concern. What should we call it?

One option is to call it soundAlarm(). It’s not quite true, however. The sounding is conditional.

We could, on the other hand, get explicit: soundAlarmIfSensorTripped(). This smacks more of implementation than intention, however. Now, having said that, I do think that in legacy code these names are great reminders as you make your first cut.. slicing and dicing a long method. They help you remember what is going on as you plan your next move.

Here’s another option: handleAlarmActions(). I don’t like this one very much. The word handle is nice, but the name makes it sound like the alarms are an event.

How about this one? handleSensorStateChange(). Much better in my opinion. I’m not quite sure how to justify this but I think that it is better to name these sorts of methods after their condition rather than their action. In the legacy code situation, at least, it helps to accent conditions. In not-so legacy code, it separates concerns at a deeper level.

Are there exceptions? Of course. Null pointer checks, guards.. the conditions that indicate errors rather than problem domain conditionality.. code surrounded by these can be safely extracted into methods named for their primary action.

This:


    ...
    if (alarm != null) {
        ...
        alarm.sound();
    }
    ...

becomes this:


    ...
    soundAlarm();
    ...

with this definition:


    void soundAlarm() {
        if (alarm == null) {
            // handle error, throw exception, etc.
        }
        ...
        alarm.sound();
    }

That code still has problems, but that extraction was a way forward.

All this is good, as far as it goes, but there is still a problem. The handle naming idiom seems to always call for generalization when I use it in real cases.

Imagine a condition like this:


if (sensor != null 
        && sensor.isTripped() 
        && configFlag == X_MODE 
        && usingBasicIO) {
    ...
}

It would be handle.. what?

Sometimes you have a good name for a condition, sometimes you don’t. A convention would be nice, however.

Posted in Michaels Musings
Meta 27 comments, permalink, rss, atom

Comments

Leave a response

Avdi Grimm about 2 hours later:

A convention I’ve seen in (IIRC) Emacs-Lisp is to use the prefix “maybe”. As in maybeSoundAlarm(). True, it doesn’t tell you anything about the condition, but at least it makes it clear that it IS conditional and, as you point out, sometimes the condition is too complex to be summed up. “Maybe” is a flag to the reader to consult the source if they need to know under what conditions the alarm will sound.
Dean Wampler about 2 hours later:

Personally, I would prefer soundAlarmIfSensorTripped over handleSensorStateChange. If I tell the Stakeholder “the alarm will sound if the sensor is tripped”, she will nod her head in agreement. If I tell her “the sensor state change will be handled”, she will mumble “WTF??”
Gaboto about 2 hours later:
What about modeling it with a state pattern or something that implies using polimorphic objects? I think the problem is that there are so many conditions that your code has plenty of if’s

For example. If you are working on something like a parser (as you showed in your provious post):
```
if (Character.isLetter(ch) && identCharCount > MIN_IDENT_LEN) {
    currentToken.append(ch);
}

if (Character.isWhitespace(char ch) {
    tokens.add(new Token(tokenBuffer.toString(), TokenType.IDENT));
    currentToken = new StringBuilder();
}
```
you can replace that code with something like:
```
...
this.parserActionFor(ch).doIt();
....
ParserAction parserActionFor(char ch)
{
     foreach( this.parserActionsClasses as actionClass)
    if(actionClass.isFor(ch))
         return actionClass.newFor(ch);  
}
...

Class ParserActionForLetter extends ParserAction
{
    static bool isFor(char ch)
        {
             return Character.isLetter(ch) && identCharCount  > MIN_IDENT_LEN;
    }

    void doIt()
    {
        currentToken.append(ch);
    }
}
```
I think that something like that is more readable and makes your code more extensible and clean. I don’t think that a name like “doSomethingIfSomething” is a problem, because it is readable and with clear intention. Nevertheless, if you have so many methods with those names in your class maybe you can try to model more polimorphic objects to solve the problem.
(Sorry about my english and my programming languages mixture in the examples :) ) bye! Gaboto
Morten Lyhr about 2 hours later:

If it is so hard to name right, perhaps we should not refactor the if statement to a new method?

On the other hand if the If statement is easily included in a method name(guard clauses etc.) then it can be refactored.

I think your if statements should be at the same level of abstraction, as the method(s) that is inside them
Christian Edward Gruber about 3 hours later:

So this whole issue stems from the fact that you’re actually building complex business rules (if chains are often a business rules issue). You might consider that this is the sweet spot for a rules engine, such as JESS, DROOLS, or JRules and the like. These use funky decision tree systems to keep rules execution clean and fast, then fire events (or hit callback functions) upon the successful exit from the rules system at a meaningful point.

If you have a small area where this is concentrated, then it’s probably overkill, but if you have lots of this sort of thing, your application may benefit from this approach. From a code aesthetics and maintainability perspective, these can really really clean up this kind of system. But it does effectively turn your system into a quasi-event-driven system, so your event-oriented naming becomes appropriate, and is quite nice to read.

in general, however, when I see if (foo && bar && (bash || !blah)) { doStuff(); } in the core business logic, I start to consider a rules engine, if it’s more than a few instances of the case.

Side note – this is sort of an application of the domain-specific language concept. You’re using a rules execution language (not a true DSL, but more domain-specific than a generalized programming language).
Mark Stijnman about 4 hours later:
The example you are using here again seems to indicate a context where many events need to be connected to some action. I was wondering: is this the context in which you encounter these most?

Let me summarize the problem as I interpreted it: you have a long list of events (conditions) and their corresponding actions that are all in a single function. The connection between each event and each action is done by a simple if-statement. You now want to refactor this function, but you want the original function to still reflect the event, the action and their relationship.
In that case, personally,I wouldn’t refactor it further than:
```
if (isSensorTriggered()) { SoundAlarm(); }
if (someOtherCondition()) { someOtherAction(); }
...
```
To me, this expresses the intent of the code just fine. An “if (sensor null)" check can be moved into "isSensorTriggerd", just as "if (alarm null)” can be moved into “SoundAlarm()”, because they are implementation details of the event and the action, respectively. They distract from the overall intent, so I want them gone. However, I want to maintain the cause-and-effect relation between “isSensorTriggered” and “SoundAlarm”, so I don’t want to move that “if” into a separate function. IMHO, moving both the event and the action into a single entity will never be satisfactory. You have basically three choices:
1. Only name it after the event. This loses information on what the action will do when the event occurs.
2. Only name it after the action. This loses information on when the action will be performed. (I suspect Avdi’s suggestion of the ‘maybe’-convention to be among the best of this type, as it at least alerts the user that this information is missing)
3. Name it after both the event and the action. This makes me feel uneasy because it means I’m admitting I have one entity performing two tasks, but at least all information is present in my main function. However,
```
IfSensorTriggeredThenSoundAlarm();
```
  or
```
SoundAlarmWhenSensorTriggered();
```
  isn’t really better than
```
if (isSensorTriggerd) { SoundAlarm(); }
```
If you really want to (or need to), you could work towards something like:
```
EventHandler ev = new EventHandler()
ev.RegisterEvent(isAlarmTriggered, soundAlarm)
ev.RegisterEvent(someOtherCondition, someOtherAction)
...
// elsewhere:
ev.HandleEvents()
```
This solution is more declarative in nature, clearly listing all the events and their corresponding actions, while more explicitly pointing out the event-action nature of the code. It also allows easy extension of the list of events and actions, even at run time.
As to your final example: it seems to me that the sub-conditions in that if-condition have different functions and therefore need to be separated anyway. Some clearly belong to the event condition (check for null on sensor: sensor can only trigger when there is a sensor), so they should be moved to the isSensorTripped() function. Others appear to belong to the action (what sort of IO to use), and should therefore be transferred to the SoundAlarm() function. I expect that most of the sub-conditions can be pushed to either the condition or the action functions.

Yet other sub-conditions appear to be used to switch the event on or off completely (configFlag). Note that the EventHandler approach allows you to elegantly separate this last type of sub-condition function as well: you simply register the isSensorTripped event only when configFlag is true.
orcmid about 5 hours later:

I first saw the desire to bury conditional code in compiler writing a very long time ago. The idea is to assure something if it is needed and do nothing otherwise (the desired state is already there), and to move the conditionality into the procedure to cut way down on having the caller know the condition to check at every point of call.

I think the key idea is that the post-condition is fixed, whatever the pre-condition is. This allows simple sequences of operations and, at the end of each, some post-condition is established.

For example assureInAccumulator(whatever) or assureInputReady(stream), that sort of abstraction.

Instead of using is… (for a predicate) or if… (with unclear assertion), I thought of handle… after the first post. You can also use neededAlarm(...) or anyAlarm(...) to avoid convoluted wording, the idea being that, if it is called for, it has been handled. Sometimes mk… or make… are used.

I think whatever it is, you want to consider the post-condition and frame the name in terms of whatever action is required to achieve it. But being creative and appropriate to the context matters.
Mark Knell about 19 hours later:

For that matter, I’ve never been enamored with the naming pattern that begins booleans with “is”. Like most of the above schemes, it’s Hungarian notation all over again: forcing metadata into the same lexical token as the name. ughYuck.

Back to the point: you’ve already isolated where things fall apart. You had a nice, compact, conventionally verb-y phrase in mind to express the method’s core behavior (“soundAlarm”). So far, so incontrovertible.

Once you start to shade the meaning with qualifiers like “if”, “on”, “maybe”, “handle”, etc., the days of simple declarative sentences are behind you. In the syntax of natural language, when a verb becomes conditional, you’ve gone subjunctive. Quick show of hands: who can explain the subjunctive tense without a side trip to Wikipedia? Okay then: who knew that, technically, the subjunctive is a mood, not a tense? How many of you non-hand raisers are effective thinkers and communicators, despite your crippling ignorance of grammatical formalisms? Yeah, I thought so.

In other words, the problem of expressing conditional behavior is not comfortably solved in most natural languages, so I wouldn’t lose sleep over it here. Moreover, by the nature of the problem, the extracted methods are implementation details, hence private, hence you can assume that no human reader will encounter any one of the extractions without the others close at hand. Any halfway consistent notation will do.
Keith Nicholas 11 days later:

considerSoundingAlarm
Daniel Rijkhof 22 days later:

After reading i can only think of doing the next:

Split your if into 2 if’s and only extract the inner if.

Very easy:

if ( configFlag == X_MODE && usingBasicIO ) { soundAlarm(); }

+ the method…

void soundAlarm(){ if (sensor != null && sensor.isTripped() ) { ... } }

You still might want to extract the outer if aswel, but naming will at least be easier…
http://www.blacktowhiteiphone4.com over 2 years later:

Bad news for folks who’ve been holding out for a white iPhone 4: Apple has delayed the white version of its popular smartphone until later this year. The white iPhone 4 was first delayed until late July, and then promised for the end of the month by Apple chief executive Steve Jobs just a week ago, but now there’s no ship date in sight.
accounting services over 2 years later:

The example you are using here again seems to indicate a context where many events need to be connected to some action.accounting services
Golda Skewek over 3 years later:

I love this one. A very good post. Thank you. roofing venice
okey oyunu oyna over 3 years later:

great explanation.

internette görüntülü olarak okey oyunu oyna, gerçek kisilerle tanis, turnuva heyecanini yasa.
real estate advertising over 3 years later:

I love your article, I read it Thank you :-)
christian louboutin shoes on sale over 3 years later:

The article is dull.
beats by dr dre headphones over 3 years later:

I found that his foot odors never bring us to death.I never regret buying these beats by dr dre studio for him. These beats by dr dre solo are just the same as selling in the franchise store.Or even better.
Bowtrol over 3 years later:

hmm ,i’m not sure if this is what i’m looking for but anyway this is interresting and could be useful some day,thanks for taking time to write such cool stuff
shoesshoes over 3 years later:

Cheap alfredia louboutin profit designed. 2010 Brand-new Design alfredia louboutin Footwear, Jimmy Choo Pompesalfredia louboutin bridalPumps, Innovative sandals resorts in jamaica, Alfredia Louboutin Boots. Excellent Promises. Jimmy ChooNo charge Travel. Get hold of Now!, canada gooseAlfredia Louboutin women’ ersus attire around ShopStyle. canada goose outlet,Glance prevalent sellers to uncover alfredia louboutin pumps for cheap profits – a lot of a particular posture.

LOS ANGELES just simply currently,

north face jakke
a person’s sizeable theaters round the usa in a hurry to the shops humorous hwy humourous training video launched, “usua
north face jakke
lly means all over, ” furnishing increase so you might witnessing a different false male or female, this actually also popular issue is certainly absolutely in the training video merely a astounding sport activity gamed outside just by Zhang Xin Yi A person’s “ garnishment hoax girl. ”
UGG Paisley Stivali over 3 years later:

UGG Paisley Stivali , Come si lascia il backspin, il difensore è in questo momento alle spalle tutti, e vorrete essere alla ricerca insieme al basket hoop.Step 3: Fine-Non appena si lascia la “spin” che normalmente hanno due alternative , sparare o forse pass.If ha avuto un fronte superiore aperto con l’obiettivo di basket (RIM), quindi prendere la shot.Nevertheless particolare, se un altro difensore ha lasciato uno in ogni dei tuoi compagni di squadra per aiutare la difesa, allora è consiglia di passare a tuo compagno di squadra ora disponibile per avere un facile cesti regalo gourmet e un superbo assistere in base alle proprie needs.Keep a cuore, se si può avere iniziato a spostare i propri Spin dal post basso, allora avrete una buona aspettano di utilizzare il tabellone per un turno facile lay-indietro up.Now che ar
ysbearing over 3 years later:

Slewing ring is also called slewing bearing, some people called: rotary support, swing support. English Name: slewing bearing or slewing ring bearing or turn table bearing, slewing ring in the real industrial applications is very wide.
Tips For Bowling over 3 years later:

I maintain also that substances, whether material or immaterial, cannot be conceived in their bare essence without any activity, activity being of the essence of substance in general. Gottfried Leibniz
alwadifa over 3 years later:

I liked you blog so im going bookmark it with my prefered websites, you have posted an amazing posts so thank you I liked you blog so im going bookmark it with my prefered websites, you have posted an amazing posts so thank you
Office 2010 over 3 years later:

This article is GREAT it can be EXCELLENT JOB and what a great tool!
mbtshoe over 3 years later:

Australia Beats By Dre Studio dr dre beats headphones beats studio beats pro beats solo hd pro headphones music Official store Monster Beats By Dre Pro
Adidas By jeremy scott over 4 years later:

Secret Adidas Jeremy Scott “Miss America” How to Make Karina Brez 24-year-old Ukrainian immigrant, from childhood with his parents immigrated to the United States, is now Wellington http://www.enjeremyscottadidas.com gem connoisseur. The 2012 Florida Miss winner is Miss America, the nation’s competition to prepare. Its preparatory activities, including a strict diet and its training, the training is almost innumerable, from the physical training, wear high heels to walk, how to answer questions, to hair, clothing and camera posture and to. http://www.enjeremyscottadidas.com/womens-shoes/womens-adidas-jeremy-scott-logo.html http://www.enjeremyscottadidas.com/womens-shoes/womens-adidas-jeremy-scott-wings-2-0.htm
beats outlet over 4 years later:

I think although bel canto, sex, delicious are different things, and touch is also different sense organs, but having a bit is with. cheap dr dre beats, beats by dre outlet.
Can bring cheerful, good psychological feeling and enjoy. beats outlet. In some cases, the aesthetic feeling of strong impact, dr dre beats outlet,can even make the person produces excited, thriller, climax…... Of course,dre beats outlet, most of the time, we just need the quietly enjoy that brings the aesthetic feeling heart cheerful. monster beats outlet.This mind the sense of joy and satisfaction, I think is that the bel canto, sex, delicious this series of several things up. beats outlet, dr dre beats cheap.
For example, now I am through Italy CC Admonitor horn appreciate a beautiful art trio of performance Clara schumann’s piano trio works monster beats outlet(schumann wife works, hardly one to hear). dr dre beats outlet,Wonderful piano, violin, cello acoustic sound, so that I vaguely recall today enjoy lunch-the "top" near wujiaochang Thai restaurant food. beats by dre outlet, beats outlet.Delicious and bel canto, also bring me a cheerful, enjoy, beats for cheap,satisfaction, although only meet different sense organs. dre beats for cheap, cheap beats, cheap beats by dre.

Mentor	twitter id
Uncle Bob	unclebobmartin
Brett Schuchert	schuchert
Michael Feathers	mfeathers
Bob Koss	bob_koss

If-Methods Redux 27

Comments

Blog Search

Follow us on twitter

Categories

Blogroll

Syndicate