If-Methods Redux 25
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.
Clean Code review on Slashdot
If-Methods 32
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();
}
The list of ifs went on and on. Each of them did something distinct and I wanted to break the method down into a set of meaningful sub-methods, but how? The typical pattern to use is to extract the bodies of the if-statements into separate methods, provided you can find good names for them. However, when I imagined what the code would look like after that refactoring, I didn’t really like it. It was still rather wordy:
if (Character.isLetter(ch) && identCharCount > MIN_IDENT_LEN) {
appendToCurrentToken(ch);
}
if (Character.isWhitespace(char ch) {
addToken(currentToken.toString(), TokenType.IDENT);
}
So, I thought that I’d extract each of the if-statements into its own method:
void onIdentChar(char ch) {
if (Character.isLetter(ch)
&& identCharCount > MIN_IDENT_LEN) {
currentToken.append(ch);
}
}
void onWhitespace(char ch) {
if (Character.isWhitespace(char ch) {
tokens.add(new Token(tokenBuffer.toString(),
TokenType.IDENT));
currentToken = new StringBuilder();
}
}
The calling code would certainly look cleaner:
onIdentChar(ch);
onWhitespace(ch);
The only problem is that the method names are a bit of a lie. When we use an “on” prefix we’re writing in event style. We expect that a method like onWhitespace
is called only when we have whitespace. It shouldn’t have to check for it.
I’ve run into this situation before. How do you name a function which consists of a single if-block? The situation crops up frequently in legacy code. You want to break up a large method into several smaller ones and you’re left with a dilemma: do you extract the entire if-statement or just its body?
Here’s another example.
if (replyFlag == RE_SEND) {
...
// code that sends a mail message
...
}
I once asked a friend what he’d name a method for that if-block if we extracted it and he said “sendEmailIfNeeded.” We both flinched and moved on to other things, but let’s think about that name a bit. Part of it talks about intention and part of it talks about implementation. Maybe we can pull the implementation bit out and concentrate on the condition:
if (needResend()) {
...
// code that sends a mail message
...
}
Better. Now, how about extracting it like this?
ifNeedsResend();
It’s sort of like the event style I wrote about earlier. The method is a handler for a condition and the condition is checked in the method. Violation of the Single-Responsiblity Principle? Perhaps, but I do know that I’m looking at some code now that has been clarified by this idiom.
public List<Token> getTokens() {
onStart();
for (int n = 0; n < text.length(); n++) {
char ch = text.charAt(n);
ifLetter(ch);
ifWhitespace(ch);
ifLeftParen(ch);
ifRightParen(ch);
ifVerticalBar(ch);
ifColon(ch);
ifDigit(ch);
}
onFinish();
return tokens;
}
What do you think?
User Stories for Cross-Component Teams 37
I’m working on an Agile Transition for a large organization. They are organized into component teams. They implement features by forming temporary feature teams with representatives from each of the relevant components, usually one developer per component.
Doing User Stories for such cross-component features can be challenging.
Type Scum 33
Getting existing code under test is hard work, but it is fruitful. Yes, you get code that is easier to change, but more importantly, you get knowledge – you learn things about programming which make you better at avoiding common traps. Sadly, many of these traps aren’t well recognized yet.
The trap that I am going to write about today is one that I call type scum. It’s most prevalent in C and C++ but it can attack in any of the traditional statically typed languages.
Type Scum is the cruft in a code base which makes it impossible to compile a single file without an entire sub-stratum of defined types. I’m not talking about the primary, or even the secondary abstractions in your system, but rather the 200 or so basic types and structs that your abstractions depend upon.
Again, the problem is worst in C++ and C. At some point, every C or C++ developer feels the urge to isolate him or herself from the basic types of the language. The unsigned int type becomes uint32 and unsigned char * becomes uchar16_ptr. And, if that was all, it would be okay. But no, people define data transfer objects which aggregate these type pseudonyms together into large muddles. No file can compile without bringing in a world of types which cushion the code from dangerous things like the platform and testing harnesses.
No wait, testing harnesses are good. What can we do?
The unfortunate thing is that it is very hard to pull type scum out of a system once it’s been infected, but we can learn how to avoid it or at least manage it better:
- If you must provide a sub-stratum of basic types in your system, do it in one place. There should be a single library (and associated headers) that you include whenever you need it. This library (and headers) should contain nothing else.
- If you must create DTO (Data Transfer Object) types, minimize them. A good general purpose structure can carry a wide variety of different types of data and simplify testing.
- Push the DTOs to the edges. There are some systems where you really do care whether an internal computation happens in unsigned long int or unsigned long long int but they are rare. Basic data types and tolerances matter when two systems need to agree upon them, and that happens at component boundaries. In many systems, the internal code can use platform types directly.
There you go. Type scum bad.
I’m sure that some people reading this will say “Hey, isn’t this the exact opposite of the advice that people give for a system with the primitive obsession code smell?” The answer is “yes.” But, to me, primitive obsession is a different problem. It’s something which is the result of a lack of real behavioral abstractions in a system, not the lack of larger data holders.
Different problem.
Type scum bad.
Configuration Management Systems, Automated Tests, CI, and Complexity 53
I’m working with a client that has a very complex branching structure in their commercial CM system, which will remain nameless. Why is it so complex? Because everyone is afraid to merge to the integration branches.
The Liskov Substitution Principle for "Duck-Typed" Languages 102
OCP and LSP together tell us how to organize similar vs. variant behaviors. I blogged the other day about OCP in the context of languages with open classes (i.e., dynamically-typed languages). Let’s look at the Liskov Substitution Principle (LSP).
The Open-Closed Principle for Languages with Open Classes 126
We’ve been having a discussion inside Object Mentor World Design Headquarters about the meaning of the OCP for dynamic languages, like Ruby, with open classes.
Tag: How Did I Get Started in Software Development 12
Micah Martin tagged me a while ago:
There will be code 27
During the last three decades, several things about software development have changed, and several other things have not. The things that have changed are startling. The things that have not are even more startling.