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

On 01/13/2015 08:25 AM, Timothy Pearson wrote:
>> -----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)
> 
> The problem with allowing these is that if I need to refactor the code to add in a single conditional I need to add
> parenthesis, which a.) is an additional source of error and b.) messes up the difference tracking making it hard
> for other developers to see what the functional change was. I think I'm going to override you on this one and force
> the parenthesis. ;-)

No problem, not a big deal.

> 
>>>> 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.
> 
> But the base class(es) are fundamentally different than the other members; I like the visual distinction on first
> glance.  Slavek, what is your opinion here?

Up to you and Slavek, both ways are ok for me.

>> 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.
> 
> While that is more compact, it introduces a problem with maintainability. Say I need to add a new member variable,
> strongly associated with m_c. That changes the definition to:
> 
> MyClass::MyClass(int i, char c, int *p) : m_i(i), m_c(c), m_cRel(c_r), m_p(p), m_d(d) { do_something(); }
> 
> which a.) required quite a bit of editing (enough to make me think twice about adding the new member variable!) and
> b.) now shows two lines as completely changed in the difference output.  The less-compact style makes it easy to
> add the variable, and shows at a glance what changed in the difference output.
> 

ok, I see the point.

Cheers
  Michele
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJUtFo5AAoJECp1t8qK3tXPMVcP/1KL33VWgHreUbeJXMcylmSY
NzOn+mqITsZ+U+Mr1HWoX7KTEfgK0JsnEED5xJqfb+MsBNB+db+H0ky2BZTFTVve
kkCKtzrVcZ11H0oR6UGLjzhXeYgv2jIgara3UhmhRfnYKoNZHShXPidFLFXWOaGS
pqjQsp/ZUkr7khnVenrli/PlpzbQtj6SdM+4ncx1sWBxQKttRdXHJUPxNZm9MBzj
3yqaBogj3hBlD0xWBUx82NlbDLMbtquwYUYDjUUCyCbNvylVdD9f6Ie9Xn72lSIV
+pqAkKwS7dmfrFjNFgLQ8W86kSDy5/n7h6J/S7xrqYP7R/bgSPPP3tma3hsda0gl
xsbPxsR29A0yQ2gno1OZ0xjjWQqxM+0pGABm9NsbYOWa/dT8VJ1jPt4Khi5305nz
00rKeBKDxbP+xfP4UVf823kFLkZQdAsBeA6sCe6A+9wK6yGLXijtcMnXg6fQzdJg
qSwYlAHk0MAXhWYXJcSNHFGttAPVjWydmzOQsAvE41AU6L+FW7Hd2sbNo83GxLQy
Jf0WhDGSixeUNj5ZMiOO8hWW1g9qCzV9aHde26Nbzxc6qt/Z3gPOIMqQjRT6VD9t
DC5vNhYrbinsWX8EUoSbd3WmuFZphzULV0FrsPZ6GjTSpaaDbMqikTM916Pfb7dk
rV2w6EJ+WU2ukpQPCKSH
=8Ljh
-----END PGP SIGNATURE-----