Re: [trinity-devel] Codebase formatting discussion

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512

<big snip>

>> This is of course subjective and not a big deal for me as well (I have already worked with both style >> actually). Mostly two reasons: 1) the operator at the end of the line tells me immediately that the >> conditional expression is not over and I need to continue reading the next line > > Wouldn't the lack of a curly brace state the same thing, given our rules on curly braces?

Yes, but an operator is more visible than a missing brace :-)

> >> 2) logically easier to read complex statement. For example > >> if (a == 3 && (b == 5 || c == 4 || (d == 10 && e == 20))) > >> rather than: > >> if (a == 3 && (b == 5 || c == 4 || (d == 10 && e == 20))) > >> I find the second one more prone to misinterpretation, since the || in the 3rd row might look at the same >> level as the && in the second row at first sight. And in TDE I have seen some rather complex and long ifs :-) >> Just my 2 cents, let's see what Slavek thinks as well. > > Point taken. However, I think we need to add a rule as well that states all conditionals must be enclosed in > parenthesis; I have cleaned up so many compiler warnings and so much buggy code becase of lazy programmers in > this respect that I don't want to see another unenclosed conditional. ;-) > > If this rule is enforced, your second example becomes: if ((a == 3) && ((b == 5) || (c == 4) || ((d == 10) && > (e == 20)))) > > which is a bit easier to read, but overall this style is still harder to read than your first example when > more than one condition is present on one line. Perhaps we should allow both styles and simply state that the > style providing "best legibility" should be used? > > The number of long/complex ifs in the codebase are why I am insisting this be hammered out properly. :-) We > haven't head from Slavek in a while so I guess we'll keep drawing up the specification and he can review it > and comment when he has time. >

Forcing parenthesis everywhere can make code more difficult to read sometimes, as in:

if ((a == 3) || (b == 5) || (c == 4)) vs if (a == 3 || b == 5 || c == 4)

So I also favor the "best legibility" rule. Let's restate it as something like "the minimum number of parenthesis that clearly isolate a conditional logic block. Example:

Right: if (a == 3 || (b == 5 && d != 20) || c == 4) Wrong: if (a == 3 || b == 5 && d != 20 || c == 4)

>> Another thing is class/struct member names. I usually add an "m_" to every member, so I know immediately that >> a variable is a class member. TDE is a very wide variety of class member names. What is your opinion? > > Yes! This is Hungarian notation for member variable scope, and should be strongly enforced. Added to spec for > new code, though I don't think we can go back and fix this automatically. > >> Another thing is class constructor parameter definition. Which one is better? > >> <style snip> > > I prefer this: MyClass::MyClass(int i, char c, int *p) : m_i(i), m_c(c), m_p(p), m_d(d) { do_something(); } > > and where a derived class is being created: MyClass::MyClass(int i, char c, int *p) : TQObject(), m_i(i), > m_c(c), m_p(p), m_d(d) { do_something(); } > > Is this acceptable?

If we use this style, for consistency I think the constructor of a derived class should be:

MyClass::MyClass(int i, char c, int *p) : TQObject(), m_i(i), m_c(c), m_p(p), m_d(d) { do_something(); }

since the "base block" is just another part of the class object, as any other member.

My only concern is that with big classes, the list of members can be quite long and so requires some scrolling from the constructor definition line to the actual constructor body. I usually use this style:

MyClass::MyClass(int i, char c, int *p) : m_i(i), m_c(c), m_p(p), m_d(d) { do_something(); }

which is somehow more compact, but I am fine with both versions.