4

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

marked as duplicate by gnat, GlenH7, Kilian Foth, user40980, Arseni Mourzenko Mar 16 '15 at 16:01

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 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"? – Kilian Foth Mar 9 '15 at 15:37
  • I'd only extract statements to functions if the code is non-obvious. – Oliver Weiler Mar 9 '15 at 16:01
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
    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 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.

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

-3

The only case where code provides better documentation than a comment is when the code is an assertion statement.

The point of writing documenting code to enforce what the documentation stipulates, or at the very least to have the documenting code refactored together with the code that it pertains to, so that it remains relevant. But the extra function does not enforce anything, and if Id gets renamed to Key one day, the name of the function will still be resetCurrentId... so it will become as irrelevant as a comment would.

So, since everything else is equal, (I shall ignore the points being made by others that the function does not seem to do what it promises, and that it works by side effect,) I would opt for the first approach by the very simple rule of minimizing lines of code: the first approach is one line of code shorter (several if you count the curly braces) and therefore preferable.

  • 3
    What about the case where a comment has been rendered inaccurate by some change to the code? It would seem to me that the code's providing better documentation in that case (which is not at all implausible). – user1172763 Mar 9 '15 at 20:34
  • Comments that tell what the code is supposed to be doing are code duplication of the worst sort. – Rob K Mar 10 '15 at 18:13
  • @user1172763 my answer anticipates and answers exactly that. You might want to read it again. – Mike Nakis Mar 10 '15 at 18:50
  • @RobK comments are not code. – Mike Nakis Mar 10 '15 at 18:51
  • If it describes what the software is supposed to do, it's code. – Rob K Mar 10 '15 at 19:26

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