Should the method describe its side effects?

Question

I was reading Clean Code by Bob Martin and there's one particular code smell, related to naming, that looks interesting to me:

N7: Names Should Describe Side-Effects
Names should describe everything that a function, variable, or class is or does. Don’t hide side effects with a name. Don’t use a simple verb to describe a function that does more than just that simple action. For example, consider this code from TestNG:

public ObjectOutputStream getOos() throws IOException {
    if (m_oos == null) {
        m_oos = new ObjectOutputStream(m_socket.getOutputStream());
    }
    return m_oos;
}

This function does a bit more than get an “oos”; it creates the “oos” if it hasn’t been created already. Thus, a better name might be createOrReturnOos.

I think that the method name should say what the method does and not how it does it (since this would break encapsulation of the implementation).

So, for me, this method does one thing. It retrieves (gets) ObjectOutputStream object as its name says and renaming it to createOrReturnOos would be revealing implementation details to the caller. And as a caller I just want to get ObjectOutputStream and I don't care about how was it retrieved/created.

If the method was private, maybe I could agree that proposed name would me more benefitial, since the method would have class visibility and strong cohesion is desirable, but since method is public, that's one more reason to abstract the name of the method.

In this situations (which happen quite often), I always have two conflicting questions: should I abstract what the method does or say in details what the method does? But, again, since this is a public method I'd rather abstract what the method does.
Also, maybe Bob found a bad example of what he wanted to say.

Your thoughts?

Answer 1

Step 1 is of course make it so your method doesn't have side effects.

But let's focus on the example. Yes, you should generally abstract what a function does from how it does it, but side effects are clearly still part of what the function is doing. By making them explicit in the name, you are making the code clearer about what is going on, leading to less bugs.

This sort of function that works with streams has special significance as well. The caller of this sort of function needs to know if the stream is new or not since that sort of thing indicates to the programmer if they need to clean up the stream or not. Managing that sort of resource ownership is a dicey scenario that leads to all sorts of nasty bugs (especially in non-memory managed languages). That makes clear communication of intentions all the more important.

Answer 2

So, for me, this method does one thing. It retrieves (gets) ObjectOutputStream object as its name says

No. You can't retrieve something that doesn't exist. Creating an object which didn't exist before and returning that object changes program state (possibly with other significant side effects). It's not a retrieval. Retrieval should be a safe, low-impact operation.

Creating a new object and handing it back changes program state. At the very least, it is adding a new object to the heap, consuming a bit more memory and using whatever cpu cycles are involved in object creation. That alone can have a significant impact on application performance if this function is called frequently.

And creating a new object may have other side effects:

• Opening a file
• Starting a thread
• Writing to a database
• Opening a network socket
• Resetting the state of other objects

Do you know how many of those an Oos may trigger on creation? If all you want (and all you are given) is a reference to the existing object, none of those need bother you.

My last point there is very important. The code snippet implies that there is only one Oos in this context. So the creation of a new Oos may wipe the state of related objects (caches, counters, stacks) and return them to a virginal condition. Oops.

renaming it to createOrReturnOos would be revealing implementation details to the caller.

If the Oos object were never returned to the caller - if it were a transient, hidden thing - that would be true. But you're returning the created object. Its provenance and history are important.

And as a caller I just want to get ObjectOutputStream and I don't care about how was it retrieved/created.

You should. Knowing which operations change state and which don't is very important.

• Some resources (e.g. databases, clustered data caches) scale by having one node which takes writes while all the others can only be read from. If you don't know which functions change state and which do not, your application will break unexpectedly.
• Some internal resources (e.g. thread pools) can be exhausted by problems with just one function/component. For resilience, it is good practice to segregate these resources into separate pools and assign potentially destructive components to separate pools, so that one component suddenly exhausting a resource doesn't starve the others. But for that to work you have to know which functions consume these resources.
• Security concerns often require you to know which functions change state and which just report it. If you have that knowledge, functions which change state can be restricted so that only privileged processes can use them.
• Graceful degradation (a desirable feature) means that your applications can still do much even if they have lost some of their resources (e.g. the master database, where writes can be performed). A well designed application can still return information even if it can't change it. But designing such code requires you to know which functions change state.

So far, I've only been talking about side effects which you may not have considered and why those are important. But there is another significant consideration.

Dishonesty.

You're lying to the caller.

The caller should expect ObjectOutputStream to return the existing stream. It may have significant state (a cache, a history of operations, output written to a file) which the caller expects to inherit, even if they don't have direct access. Streams in particular tend to have a lot of state and history. Well, you lied to them. Here is a brand new object without that state. They don't know that. You shouldn't lie like that.

Maybe there should be an existing Oos in the current context. The lack of one may mean something died or a bug elsewhere in the code deleted the old Oos. Now you hid that error. Debugging this is going to be fun.

You're also denying the caller the freedom to create his own control flow. Now, I'm a FP kind of guy and I am suspicious of exceptions, but we're in Java land here. It is a common Java idiom to do things which may or may not break and leave catch blocks to deal with those cases where it does. Sometimes a whole code path lives only in there. You're denying the caller that option. And other, simpler things.

Do not automagically create things which were not there when you were not asked to. If the caller asks for something which isn't there, tell him. Let him decide what to do next.

Stop lying to him.

maybe Bob found a bad example of what he wanted to say

He did not. Clean Code has some of the best writing on software development that I have ever read and there's nothing in the section you quote that I would complain about.

Answer 3

But you do care about it being created, because it can throw an IOException!

In fact, the only time it can throw is the first time it's called. Every other time you'll need to put a pointless try/catch block (assuming this is Java) in the calling code for something that will never happen.

I'd refactor this code to have an explicit stream creation method. Having I/O happen in a getter is generally not good if the I/O ever goes wrong.

(If the object creation didn't involve I/O, I'd agree with your assessment)

Answer 4

Yes, I think method name should describe side effects.

For one thing, this example uses checked exception, which by itself reveals implementation details. Invocation of this method requires try/catch block anyway (for no reason, since it can be handled inside and throw NPE or return null, which wouldn't improve the situation, but at least make it cleaner on a caller's side), so a clearer method name wouldn't hurt.

I'd rather called it getSingletonOos, however, since it is what it actually does. getOos sounds like a plain old getter, and it is very misleading. Compare:

// Plain getter, no side effect.
public ObjectOutputStream getOos() {
    return m_oos;
}

// Additional side effects are described by the name.
public ObjectOutputStream getSingletonOos() throws IOException {
    if (m_oos == null) {
        m_oos = new ObjectOutputStream(m_socket.getOutputStream());
    }
    return m_oos;
}

The second example does more than the first one, and it should be reflected in the name. How else would you tell the difference?

Answer 5

As others have pointed out, your method does actually expose the fact that it is more than just a getter, since it publicizes the fact that it may throw an exception. So, you might as well make it clear in the name of the method that it works either as a getter or as a factory, otherwise someone using at it is bound to frown upon the fact that an innocent-looking getter is capable of throwing an exception.

In alternative scenarios which are not complicated by publicly declared exceptions, it goes like this:

The question of whether to publicize the side effects of a method is answerable not by looking at that single method, but by taking into consideration the object as a whole.

The fact that a method has side effects should be publicized if these side effects are in any way, shape, or form visible through the public interface of the object to which the method belongs.

So, if your object manages to fully hide the fact that some of its methods internally performs side effects, then it is fine to not publicize this fact in the name of any public method. However, if, by invoking other methods of the object, you can discover the fact that side effects took place, then those methods that cause these side effects should indicate so in their names.