5

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);
    }
}
2
  • 6
    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"? Commented Mar 9, 2015 at 15:37
  • I'd only extract statements to functions if the code is non-obvious. Commented Mar 9, 2015 at 16:01

7 Answers 7

3

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.

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.

1
  • 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
    Commented Mar 10, 2015 at 6:48
3

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.

2

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.

2
  • "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
    Commented Mar 9, 2015 at 15:48
  • @Adam - no. That's probably not okay.
    – Telastyn
    Commented Mar 9, 2015 at 15:51
2

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.

0

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 );
}
0

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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.