doSomethingIfCondition(). Is it good naming?

Question

Suppose, I have some method that performs some job. In this example, it's going to be void, but it doesn't matter

public void doSomething() {
    // doing something
}

But then, let's imagine, I got my team mates complaining that my method, on occasion, doesn't actually do "something". It's because it includes some condition check. It could be a state check or input validation, but since my sample method doesn't take any arguments, let's assume it's the former

public void doSomething() {
    if (isCondition()) {
        /* 
           I guess I could ask a separate question 
           on whether doDoSomething() is good naming
        */
        doDoSomething()
    }
}

I have two options:

1. Document it
2. Incorporate that information in the method's name

I recall from Bob Martin's Clean Code that comments should always be seen as a failure — a failure to write code that explains itself. Obviously, I don't want to fail (I want to win, win, win). So I opt for the latter option

public void doSomethingIfCondition()

Then, another change. For whatever reason, I now have to check one extra condition. Since I don't want my method to lie (lying methods is a big no-no in clean code), I now have to change the method name again

public void doSomethingIfConditionOneOrConditionTwo()

That's ugly. But suppose I can somehow group those conditions semantically

public void doSomethingIfSuperCondition() {
    if (isSuperCondition()) {
        doDoSomething()
    }
}

private boolean isSuperCondition() {
    return isConditionOne() && isConditionTwo();
}

A spec overhaul. Now, neither "condition one" nor "condition two" matter one bit. It's "condition three" that really does. But here's the problem: my method became quite popular. There are a ton of clients, and it's not feasible to fix them. Or, perhaps, I don't even control all of my clients anymore. It became opensource or something. So the best I can come up with is this

/**
  @deprecated use {@link #doSomethingIfConditionThree} instead
*/
public void doSomethingIfSuperCondition() {
    if (isSuperCondition()) {
        doDoSomething()
    }
}

public void doSomethingIfConditionThree() {
    if (isConditionThree()) {
        doDoSomething()
    }
}

I can keep making this story up. The point is, I start to doubt that naming convention. I want to write clean, but trying to do so, in my fictional example, resulted in a pretty nonsensical code. I am not at all sure it's what Bob Martin intended

So, what do you think?

Answer 1

Code is full of conditionals. You can't specify the conditionals in all the symbol names!

If you find yourself tempted to include a condition in the name, it's possible your name is too low-level. There is probably a higher-level way of looking at the activity that would result in a name without the condition.

Instead of

void CreateDirectoryIfItDoesntExist(string path)

Try

void EnsureDirectoryExists(string path)

Or instead of

void StartIfReady();

Try

void TryStart();

Or instead of:

void UseRedIfEmergercy();

Try:

void ApplyPriorityColorLevel();    

Or instead of:

void WriteToLogIfDebugEnabled(string message);

Try:

void LogDebug(string message);

Answer 2

Here's the appropriate prefix:

bob.jump();

What bob does when told to jump is up to bob. Bob could jump. Bob could ask you "how high?". Bob could ignore you completely. Jump isn't about what happens. Jump is a message you sent to bob. What bob does with it is up to bob.

Whenever you find yourself mutilating a name by filling it with implementation details understand you are harming abstraction. A name should tell me why to call it. Not what it does. Telling you what it does is not a names job. Name things that way and you wont have to rename them when you refactor.

Remember, a name is a comment. Comments should tell you why the code exists. Not what it does.

---

This answer has received a mixed reception and I can see it deserves some revision. Sorry it has taken so long. Family has its needs.

Let me ask this^*:

ConfiguredWithNoFileSystem.EnsureDirectoryExists(somePath);
SystemThatNeverStops.TryStart();
BlackAndWhiteDisplay.ApplyPriorityColorLevel();

Isn't the Null Object Pattern something that could be useful here? Isn't silently doing nothing ok when that's what is really needed?

^{* I didn't include LogDebug(); in this list because silently doing nothing when so configured is something people have already accepted it doing.}

I absolutely agree with John Wu's point that a higher level of abstraction can free you of details that might clutter a name. He gives many good examples that communicate the expected post condition of the sunny day path. But permit me one nit pick. TryStart() is not a higher level of abstraction.

It is only announcing what you should already know. That this method might not do what you ask it to do depending on how it is configured. You should expect that even if it doesn't throw, even if it doesn't return an error, even if it doesn't report the issue thru some output port, it might do nothing, and silently ignore you. There is no method that could never end up needing to be configured to behave in this way.

The reason I feel that way is because the most recent knowledge always wins. Maybe in the past we were sure this will never be needed. Then time passes and it turns out we were wrong. Our job isn't to predict the future. It's to design systems that make it easy to deal with it.

I've taught this lesson for years, and I told a story that illustrates it in the middle of this somewhat old answer of mine.

The hard principle behind this idea is that reality doesn't care what you think. This is why we don't use composite natural keys in a live database. You may think combining phone number and social security will always ensure a unique id but all anyone has to do is add one more row and suddenly it doesn't work anymore. No, do that with dead data that you're trying to structure. And soon as you can make a unique key that isn't used for anything else besides being a unique key. That way reality can't mess with you.

Answer 3

The method's job is to do something.
If that something is only done based on condition(s) known only to the method, then so be it. The caller does not need to know or care about them.

If they did, then they would be testing the preconditions themselves ...

if( condition ) 
   doSomething(); 

... which makes for even messier code or passing the conditions as arguments to the method for evaluation there.

public void doSomething( bool condition ) 

(Incidentally, this approach would make the addition of conditions 2 and 3 clearer, because you'd need a new method for each).

It seems to me that the method is working just fine.
The problem is that the Developers aren't aware of these internal conditions. The way to communicate such things to Developers is through the Documentation.

Answer 4

An appropriate name is

trySometing()

Method should inform client, that it does not guarantee the named side-effect, but it should not reveal any implementation details.

This pattern is usually associated with exception handling, but IMO it is useful without those too.

Answer 5

You tend to find these naming and placement difficulties arise from a somewhat awkward conceptualisation in the first place, or from an unsound division of labour between the person who writes the method and the person who calls it, which leads to the callers using the method with an unsound understanding of what it does.

With these kind of methods which self-check, and which silently and unexpectedly become a no-op under certain circumstances, what it tends to imply is that a condition should be checked at the call site before the main call occurs and that the caller should be in charge of that check.

The reason there is a deviation from this pattern and there is an attempt to fold the check into the called method, is either because the check has to be made annoyingly frequently at different call sites, or because developers keep forgetting to make the check and the code runs incorrectly for that reason.

The problem is, having folded a silent check into the method, your developers are expressing surprise about the occasional no-op behaviour. That suggests they should be rehearsing the check themselves explicitly in code at the call site, because they (apparently) need to know that the check is occurring and are tending to forget that it does.

If the checking code is itself complicated, then there is nothing wrong with you providing that as a checking method separately from the main method, but the caller still has to go through the pattern of making the check then deciding whether to make the main call, and therefore cannot forget the possibility that the main call (and the work associated with it) does not occur.

If necessary, the check can be performed redundantly inside the method, throwing an invalid op if the call has occurred in the wrong circumstances.

However, if you stick with the existing code structure, then DoSomethingIfCondition is as good a name as any.