Single statement non-braced ifs and code merges

Question

I've seen code merges being used as an argument for bracing even single statement ifs. For example:

if (condition) {
    do something;
}

Unfortunately I can't think of a change that would break a non-braced version during an automatic code merge. Could someone post an example?

score 9 · Accepted Answer · answered Dec 01 '12 at 17:25

I think the condition they're talking about is this

Change 1 (add, line 20-21):

if (condition)
    doSomeStuff();

Change 2 (add, line 20-21):

if (condition)
    doOtherStuff();

I would bet that some old auto-merge tool somewhere would try to be clever and say "well, you've both added the same line twice at 20, so I'll add it once; and you've added two different lines at 21, so I'll add them both." Leading to

if (condition)
    doSomeStuff();
    doOtherStuff();

These historic arguments tend to hang around long after every merge tool on the planet has been fixed to work differently.

Any auto-merge I've ever seen would refuse to merge two adds in the same place, meaning you have to use a manual-merge tool. At worst, it might add one before the other, which is still fine. But a decent merge tool will allow you to edit this in place and end up with

if (condition) {
    doSomeStuff();
    doOtherStuff();
}

score 8 · Answer 2 · answered Dec 01 '12 at 16:01

8

You're right, and it won't break in the way described. Unless you're using a really, really bad merging tool, the scenario you describe shouldn't happen. Merge tools don't magically insert code that doesn't exist in either the new or old version of the file.

The real issue is that programmers are much more apt to extend the logic within a conditional and forget to add the braces. It's not a merge tool issue, it's a people issue. And I count myself in that camp having made that mistake before.

Whether all single line conditionals should be within braces is a sub-theme within the holy wars of code style. What is right can only be decided by your team for your team.

Your team needs to make a decision on what they want and put it in your coding style guidelines.

answered Dec 01 '12 at 16:01

5

I don't think it's really an issue - it's just simply that anyone who writes an unbraced if() deserves be swiftly killed by an angry mob. – Martin Beckett Dec 01 '12 at 17:46
@MartinBeckett I'm with you 90% of the way. But is it evil to leave out the braces if the statement fits neatly on a single line? if (x > 2) i++; – GlenPeterson Dec 01 '12 at 21:26
2

@GlenPeterson - the famous, "but it was only a small change....." If you need a very small if I tend to use the ternary operator. int x = test() ? 1:0; – Martin Beckett Dec 02 '12 at 01:33

score 2 · Answer 3 · answered Dec 01 '12 at 17:40

That's is so easy to break in a way that you will not spot when it does break.

 if (condition)
     doSomeStuff(A);

 // Code

Based on a real change I saw a new hire do:

-    void doSomeStuff(int A)
-    {
-        Action1(A);
-        Action2(A);
-        Action3(A);
-    }
+    // I modified this function to a macro to make sure it inlines and
+    // thus we get s speed increase due to decreased functions calls.
+    #define doSomeStuff(A)        Action1(A);       \
+                                  Action2(A);       \
+                                  Action3(A);

Yes it is a stupid situation that will not do what the comments say. But people make all sorts of assumptions and will change code based on these assumptions. And quite easily break the code.

And you will never see at the usage points it is broken until it is too late.

Thus I habitually force myself to always use braces.

This has nothing to do with code merges, but it is a fun example of why you should really brace one-line blocks. Also, there should be a rule in programming -- there's always a way to screw things up using #define. — pdr, Dec 01 '12 at 18:23

score 1 · Answer 4 · answered Dec 01 '12 at 14:54

The only thing that immediately comes to my mind, is the typical problem of a developer believing that their statement is still part of the condition when it is outside the condition therefore their conditional statement gets run. I have seen this over and over.

Example

if (condition)
    do something in condition;
    do something else in condition; // Outside the condition always gets run

I know it is not directly related to your question but if you are doing automatic code merges this is the type of thing that might sneak in more often than not. doubly so if you are not properly testing for breaks before the merge.

For this reason I feel it is good policy to be more explicit with your conditional bracing and more so if you are doing automatic merges. I also hope that with each merge there are a suite of test being run to help ensure that the merges were successful and your application still works as expected.

Single statement non-braced ifs and code merges

4 Answers4