6

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?

Improve this question
21
  • This could fall under "Tell Dont Ask". "DoSomething best way you see fit and dont bother me with condition details" Commented Sep 13, 2024 at 8:33
  • 3
    Duplicate of How to name a method which may or may not perform an action depending on a condition? Commented Sep 13, 2024 at 8:45
  • How do clients of your code know about the check? How do they know, that method results in noop? Commented Sep 13, 2024 at 10:26
  • How about ifConditionDoSomething()? Or better yet, if (condition()) doSomething(); This doesn't need to be a separate function; you're just bloating your code. Commented Sep 13, 2024 at 14:48
  • 2
    This question is similar to: How to name a method which may or may not perform an action depending on a condition?. If you believe it’s different, please edit the question, make it clear how it’s different and/or how the answers on that question are not helpful for your problem. Commented Sep 13, 2024 at 15:29

5 Answers 5

Reset to default
13

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);
Improve this answer
1
  • 1
    Good list. I might also add using return types and doc strings to communicate conditional details specifically. For example, instead of void startIfReady(), bool start() with a doc string, "returns true if successfully started, false otherwise." Commented Sep 13, 2024 at 15:03
4

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.

Improve this answer
26
  • 1
    @candied_orange, "Bob could ignore you completely." - I don't think any programmer reasonably expects the computer to ignore the programmer! That statement is too hardline. The issue here seems to be that the check (and potential no-op) is considered astonishing by the callers, rather than within the terms of what they would expect the call to do based on their broad understanding. If you call x.increment(), you don't reasonably expect there to be a hidden check where the increment silently fails if x = 42. Commented Sep 13, 2024 at 12:38
  • 1
    @candied_orange, that's a fair counter-example, but you get my point that callers expect methods to do certain things and not do other things. What those expectations are may vary in nature and in how specific they are, but callers expect the effect of a method to correspond to their understanding of what that method does - nobody expects a call to have consequences that are completely random or unknown. Commented Sep 13, 2024 at 13:55
  • 1
    @Steve random speaks to nondeterministic behavior which can be a very bad thing. But unknown behavior from the callers point of view can absolutely be a requirement. Stop insisting on knowing everything. Let something else deal with it. Commented Sep 13, 2024 at 15:30
  • 1
    @candied_orange, of course you don't have to know every single detail, but there is still an irreducible amount you must know about what each call does, in order to successfully design (or later readapt) the program for its purpose. Commented Sep 13, 2024 at 16:15
  • 1
    @candied_orange, but somebody has to understand what is going on, in order to perform the essential design work - in order to make basic decisions such as whether a method should even be called, or in what order. If every call is like "open Mystery Box X", the obvious question is why such a call is even necessary and how it came to exist in the program. It's not necessarily about knowing every detail - but there is still some kind of constraint, some kind of detail (which may differ from case to case), some kind of shared understanding with others, on which a developer must be able to rely. Commented Sep 13, 2024 at 23:25
2

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.

Improve this answer
1
  • 1
    "The way to communicate such things to Developers is through the Documentation." - everything sounded good up until this point. Commented Sep 13, 2024 at 8:47
2

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.

Improve this answer
7
  • 1
    Thank you. But then pretty every method would have to be prefixed with "try". Does it worry you? Commented Sep 13, 2024 at 9:17
  • 1
    @SergeyZolotarev only the ones that have conditions Commented Sep 13, 2024 at 9:22
  • @Caleth hardly any method doesn't (or shouldn't) perform any checks at all (except for accessors, maybe) Commented Sep 13, 2024 at 10:08
  • 1
    This is not about conditions, it is about postconditions. When the postcondition is a potential noop, but the action implies a stronger postcondition in a natural language, we can modify the name with "try" prefix to soften the implied postcondition. Commented Sep 13, 2024 at 10:09
  • If a method is called for its (useful) side effect, and that side effect occurs conditionally (I'd still argue hardly anything is unconditional at all), then its name should be prefixed with "try". Did I understand you correctly? Commented Sep 14, 2024 at 15:39
2

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.

Improve this answer
4
  • If the check always has to be done, there is no reason to burden clients with it. Commented Sep 13, 2024 at 8:49
  • @Basilevs, that's not true. The issue here seems to be that the client developers need to know that the specific check is being done or that the execution is conditional. The OP isn't clear why in this case, but it's basically a case of important but non-obvious details being nested too deeply in code. Commented Sep 13, 2024 at 10:06
  • Clients should not need to know that. The problem lies not with the method in question but with other related APIs . Probably some other ill-defined contract requires an action to be done, but fails to check if it was actually done or to do it itself, or to throw an informative exception. Commented Sep 13, 2024 at 10:23
  • @Basilevs, I agree, there is often another connected problem in these cases. Commented Sep 13, 2024 at 10:38

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.