Creating one-line method for the single purpose of self-documenting code

Question

Given the following two snippets:

Snippet 1:

public void foo(Data data, AbstractNode node)
{
    int originalId = data.getCurrentId;
    node.execute(data);

    //If node changed currentId reset it to the original.
    data.setCurrentId(originalId);
}

Snippet 2:

public void foo(Data data, AbstractNode node)
{
    int originalId = data.getCurrentId;
    node.execute(data);

    resetCurrentIdIfChangedByNode(data, originalId);
}

private void resetCurrentIdIfChangedByNode(Data data, int originalId)
{
    data.setCurrentId(originalId);
}

In both cases it is not be known whether node.execute() changes the currentId or not.

Does the one-line method in Snippet 2 make the code more readable? Or is it preferable to use comments as in Snippet 1?

EDIT: I understand that node has side effects. This is why I'm resorting to creating a method that explains (or tries to explain) that the data.currentId might be changed by node. Depending on which subclass of AbstractNode is being used, it might be changed or not.

EDIT 2: The reason there is no if statement around the reset in Snippet 2 is that it would be pointless, since the result would be the same whether we check or not.

private void resetCurrentIdIfChangedByNode(Data data, int originalId)
{
    data.setCurrentId(originalId);
}

same result as

private void resetCurrentIdIfChangedByNode(Data data, int originalId)
{
    if(data.getCurrentId() != originalId)
    {
        data.setCurrentId(originalId);
    }
}

Both your comment and your method name talk about "If node changed...", but the action is unconditional. That is very strange. Do you in fact mean "reset, **in case** it was changed by the node"? — Kilian Foth, Mar 09 '15 at 15:37
I'd only extract statements to functions if the code is non-obvious. — Oliver Weiler, Mar 09 '15 at 16:01

score 8 · Answer 1 · answered Mar 09 '15 at 15:38

8

I'd say that the method name is actively misleading. After all, it always sets the ID, not only if node changed it!

To method 1, I'd add a comment explaining that node.execute(data) can change the ID, and why it is necessary to keep the old one.

Thirdly, consider making this a method on either node or data. Can't tell from this snippet whether that makes sense, but it might.

answered Mar 09 '15 at 15:38

RemcoGerlich

3,280
18
22

1

I think it is not too unlikely that making `foo` a method of node or data may both not be feasible. `AbstractNode` looks like an abstract class with interface functionality (thus not to be changed), and it already has a dependency on `Data`, so adding a method to `Data` depending on `AbstractNode` would introduce a cyclic dependency. See my answer below for a better solution to avoid the comment. – Doc Brown Mar 10 '15 at 06:48

score 3 · Answer 2 · answered Mar 09 '15 at 15:40

The node.execute call is not a side-effect free function and you should probably change the implementation and avoid the need to reset the id altogether.

If for some reason you can't, the second version has a very confusing method name as it implies that method actually checks if the id was changed by the node, which is doesn't. So simply use the first example but add a comment that explains the execute might have changed the id and you need to keep it.

Doc Brown · Accepted Answer · 2015-03-10T20:00:38.627

Your comment can be easily misinterpreted, as you see from some of the other answers. Most devs are trained to associate the word "if" with a conditional branch, but that is obviously not what you meant.

I guess this is what you really meant:

public void foo(Data data, AbstractNode node)
{
    int originalId = data.getCurrentId;
    node.execute(data);

    //in case "execute" changed the current id of "data", restore the original one
    data.setCurrentId(originalId);
}

If you want to make the comment obsolete by picking good function names, refactoring the last two lines into a separate method is IMHO not a good approach. Instead, use a better name than foo, for example:

public void executeWhileKeepingCurrentId(Data data, AbstractNode node)
{
    int originalId = data.getCurrentId;
    node.execute(data);
    data.setCurrentId(originalId);
}

Now the intent is clear without the need for a long winded comment.

score 2 · Answer 4 · answered Mar 09 '15 at 15:38

Does the one-line method in Snippet 2 make the code more readable?

A little. The problem is that the new name is very tightly coupled to this specific code. Surely there are other reasons to reset the id to the original?

Or is it preferable to use comments as in Snippet 1?

No, that comment is worthless. You might put into a comment about why execute might change the id, or why you're resetting the id in the first place. Comment why, not what.

It's always best to eliminate the weird code in the first place, but how to do that will vary from situation to situation; and might not always be possible. If it's not possible, functions are preferable to comments, since you'll often need this sort of functionality in more than one place. Copy/pasting comments around is no good.

"he problem is that the new name is very tightly coupled to this specific code." This is my point really. Is it OK to create a method with a documenting name just for a specific case? — Adam, Mar 09 '15 at 15:48

score 2 · Answer 5 · answered Mar 10 '15 at 00:53

I'm answering the point of the broader question and not focusing particularly on this particular implementation:

comments are bad. See the thousands of questions and answers here about that. Just search.
well named methods are better than comments
extracted methods are much easier to unit test
extraction at this point will allow for any locally scoped changes you then want to make.
if the extracted method is called from more than one place this will remove the duplication.

The question though is when is enough?

For that I would focus on whether this really an atomic operation and whether it's only done in this piece of code. You can go 'too extreme' in this operation and turn a simple 10 lines of code into 8 different methods with different nesting levels that is really hard to read.

score 0 · Answer 6 · answered Mar 10 '15 at 18:14

I like the general idea. Instead of depending on a comment, which can be deleted or not updated when things change, making what's happening an explicitly named function explains what's happening and prevents the explanation from being lost.

Though as others have mentioned, I'd name it differently, perhaps resetDataToOriginalID(), and being a C++ programmer, I'd probably use a RAII based solution to provide exception safety if needed. Something like this.

class DataIdGuard
{
    int    m_originalID;
    Data & m_data;

    public:
    DataIdGuard( Data & data ) : m_originalID( data.getCurrentId() ), m_data( data ) 
    { }

    ~DataIdGuard()
    {
        m_data.setCurrentId( m_originalID );
    }

};

void foo( Data & data, AbstractNode & node )
{
    DataIdGuard guard_data_id( data );
    node.execute( data );
}

user949300 · Answer 7 · 2015-03-10T22:12:29.213

I would give the variable a ridiculously long name to make everything clear

public void foo(Data data, AbstractNode node)
{
    int savedOriginalIdBecauseNodeDotExecuteMightChangeIt = data.getCurrentId;
    node.execute(data);

    data.setCurrentId(savedOriginalIdBecauseNodeDotExecuteMightChangeIt);
}

On further thought

Something about this code really bothers me. You take care to reset the currentId of the data, in case node.execute() changes it. What else might get changed? How do I know that other properties like data.name or data.foobar didn't get changed? This is a big can of worms which indicates a code smell and possible need for refactoring.

Creating one-line method for the single purpose of self-documenting code

7 Answers7