trinity-devel@lists.pearsoncomputing.net

Message: previous - next
Month: January 2015

Re: [trinity-devel] Codebase formatting discussion

From: "Timothy Pearson" <kb9vqf@...>
Date: Sun, 11 Jan 2015 15:20:50 -0600
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA224

> <snip>
>> My only remaining question then is should we also be forcing whitespace
>> between each case block for legibility,
>> something like this?
>>
>> do_something(); switch(foo)  { case bar: a=1; break;
>>
>> case baz: { a=2; ...long case block... c=4; break; }
>>
>> case asd: a=3; break;
>>
>> default: a=0; } do_something_else();
>>
>
> Empty lines between cases makes reading a little easier IMO. Sounds like a
> good idea to me.

OK, agreed and added to official spec.

> <snip>
>>> if ((disktype & TDEDiskDeviceType::CDROM) || (disktype &
>>> TDEDiskDeviceType::CDR) || (disktype &
>>> TDEDiskDeviceType::CDRW))
>>
>>> or
>>
>>> if ((disktype & TDEDiskDeviceType::CDROM) || (disktype &
>>> TDEDiskDeviceType::CDR) || (disktype &
>>> TDEDiskDeviceType::CDRW))
>>
>> I am curious as to why you find this more intuitive than having the
>> logical operator before the comparison.
>> Roughly translating to English the above style reads something like:
>>
>> if the box is black AND <pause> the box is not square AND <pause> the
>> sphere is purple OR the sphere is NOT PRESENT
>> THEN
>>
>> versus
>>
>> if the box is black <pause> AND the box is not square <pause> AND the
>> sphere is purple <pause> OR the sphere is NOT
>> PRESENT <pause> THEN
>>
>> You can see how in the latter style the relevant logical operator is
>> stated first, then the expression, with all
>> required information to understand the conditional contained on one line
>> instead of spanning two lines.  The latter
>> is easier for me to grasp all of the conditions required in a large
>> expression; the former requires a bit more
>> effort.
>>
>> It isn't a big deal either way for me, and I'd like Slavek's input
>> before deciding, but I am curious as to the
>> thought processes behind the suggested style. :-)
>>
> 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?

> 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.

> 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?
>
> MyClass::MyClass(int i, char c, int *p, double d) : m_i(i), m_c(c),
> m_p(p),
>     m_d(d)
> {
>   do_something();
> }
>
> or
>
> MyClass::MyClass(int i, char c, int *p) :
>   m_i(i), m_c(c), m_p(p),
>   m_d(d)
> {
>   do_something();
> }
>
> or
>
> MyClass::MyClass(int i, char c, int *p)
>   : m_i(i), m_c(c), m_p(p),
>     m_d(d)
> {
>   do_something();
> }
>
> or
>
> MyClass::MyClass(int i, char c, int *p)
>   : m_i(i),
>     m_c(c),
>     m_p(p),
>     m_d(d)
> {
>   do_something();
> }
>
> or
>
> MyClass::MyClass(int i, char c, int *p) :
>   m_i(i),
>   m_c(c),
>   m_p(p),
>   m_d(d)
> {
>   do_something();
> }

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?

I have also added a question to the Etherpad regarding loop control
statement formatting; if you could respond to that I'd appreciate it.

Thanks!

Tim
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iFYEARELAAYFAlSy6R4ACgkQLaxZSoRZrGG1FADeOvjxj6sOs6VaFsw4npnSz3K0
T9Y/371klCTolADbBzS2omXHMkwCBdnWGKth+mlOwEiHOPnBrFTOZQ==
=nOB9
-----END PGP SIGNATURE-----