Vega-strike-engine-source: Code Formatting Structural Changes

[BenjamenMeyer]

NOTE: This is a list of things we should look for and clean up in the code. We need to add these to our coding standards. This is just the start of the list, more things may be added as we go along.

Code Formatting and structure are often the source of hard to find bugs. Some things that can help which we should do:

• [ ] Any C/C++ code block no matter how little (1 line is enough) should be wrapped in braces:

// not acceptable
if (5 == X)
   doSomething()

// acceptable
if (5 == X) {
   doSomething()
}

Reason: C/C++ is not white space delimited and often a second line is meant to be in the block but the lack of braces means it is part of the outer block instead of the inner block; or something was done as a side-effect of the if block and the block wasn't suppose to have a body but the next line gets caught because of the lack of a semi-colon.

• [ ] Unless absolutely necessary (which is nearly never) do not do assignments in boolean tests for loops or if blocks, etc. It is better to separate them out on a separate line for more predictable behavior and easier debugging. If it cannot be avoided, then put a comment with the block explaining why.

// not acceptable
while (x = getSomevalue()) {...}
if (y = getSomeValue()) {...}

// acceptable
x = getSomeValue();
while (x) {
   ...
   x = getSomeValue();
}
y = getSomeValue()
if (y) {...}

Reason: These tend to lead to hard to find bugs as the assignment in the condition block has yields a side-effect. It is better to make it clear what is desired to be done by separating them out. This also helps folks know that only == should be in the conditional.

• [ ] when possible, unassignable things should be on the left side of a boolean equation:

// not acceptable
if (X == 5) {...}

// acceptable
if (5 == X) {...}

Reason: By having something that cannot receive a value on the left side (lvalue) of the boolean it helps us find places where values got assigned accidentally in the if block because of a lack of double equals.

[royfalk]

I'm fine with the first two, although I sometimes favor one liners where more elegant.
However, I disagree with yoda conditions if(5==x)

Compilers and IDE warn against it. Now that we have 'treat warning as errors' (I think), I doubt this is even relevant. Also, Kate Gregory is against it, which is good enough for me.

---

From: Benjamen Meyer [e-mail]
Sent: Tuesday, August 4, 2020 4:58 PM
To: vegastrike/Vega-Strike-Engine-Source [e-mail]
Cc: Subscribed [e-mail]
Subject: [vegastrike/Vega-Strike-Engine-Source] Code Formatting Structural Changes (#223)

NOTE: This is a list of things we should look for and clean up in the code. We need to add these to our coding standards. This is just the start of the list, more things may be added as we go along.

Code Formatting and structure are often the source of hard to find bugs. Some things that can help which we should do:

• Any C/C++ code block no matter how little (1 line is enough) should be wrapped in braces:

// not acceptable
if (5 == X)
doSomething()

// acceptable
if (5 == X) {
doSomething()
}

Reason: C/C++ is not white space delimited and often a second line is meant to be in the block but the lack of braces means it is part of the outer block instead of the inner block; or something was done as a side-effect of the if block and the block wasn't suppose to have a body but the next line gets caught because of the lack of a semi-colon.

• Unless absolutely necessary (which is nearly never) do not do assignments in boolean tests for loops or if blocks, etc. It is better to separate them out on a separate line for more predictable behavior and easier debugging. If it cannot be avoided, then put a comment with the block explaining why.

// not acceptable
while (x = getSomevalue()) {...}
if (y = getSomeValue()) {...}

// acceptable
x = getSomeValue();
while (x) {
...
x = getSomeValue();
}
y = getSomeValue()
if (y) {...}

Reason: These tend to lead to hard to find bugs as the assignment in the condition block has yields a side-effect. It is better to make it clear what is desired to be done by separating them out. This also helps folks know that only == should be in the conditional.

• when possible, unassignable things should be on the left side of a boolean equation:

// not acceptable
if (X == 5) {...}

// acceptable
if (5 == X) {...}

Reason: By having something that cannot receive a value on the left side (lvalue) of the boolean it helps us find places where values got assigned accidentally in the if block because of a lack of double equals.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com/vegastrike/Vega-Strike-Engine-Source/issues/223, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACQWBEAIL4MK4VXBVYUFVZ3R7AHZ3ANCNFSM4PUNV5VA.

[BenjamenMeyer]

@royfalk I haven't seen a compiler warn against the Yoda condition. There's no reason they should as it's a valid conditional test. The point of them is to point out where such things are not valid so it's fixed instead of being a silent, hard to find error.

That said, as long as we don't do assignments inside the conditionals it's not as big of an issue. I've been mixed on them, but it is generally considered a best practice.

[stephengtuggy]

I will have trouble getting used to "Yoda conditions." I've done it the other way for so many years. But I see your point, @BenjamenMeyer , so I will give it a try.

I agree with the other points, for sure. Also, can we say something about using function/method calls on the left side of an =? This code:

    cockpit->GetUnitFileName() = newFileName;

is really hard to figure out. Like, what is it even doing? Is it valid code, or not?

[BenjamenMeyer]

@stephengtuggy agree on the function example you gave ....that really shouldn't be done. I'll update for that later

[BenjamenMeyer]

This is on-going to adding 0.8.x and 0.9.x targets.

[stephengtuggy]

So I've tried to implement Yoda conditions. Can't seem to get used to them. Can we revisit that discussion at some point?

Also, perhaps we could add automated code scanning rules for if blocks without braces; single equals signs inside conditionals; etc.?

[stephengtuggy]

One more thing: Accessing the elements of an STL collection using the array indexing operator [ ] does not check whether the given element is actually within the bounds of the collection. On the other hand, .at() does. So we might want to either use .at() everywhere, or implement custom collection subclasses that do check for boundary conditions in all cases. What do you guys think?

[BenjamenMeyer]

I'd love to have a code style scanner....harder with C/C++ to find one. Even GitGub's tooling doesn't do so for C/C++ (only major language missing).

[BenjamenMeyer]

as we're getting ready to release 0.7.0 and we want a fast turn around on 0.8.x - closing this for those two