By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
455,906 Members | 1,387 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 455,906 IT Pros & Developers. It's quick & easy.

Makeup for if-statements?

P: n/a
I have made this:

if (parent[x] == &N::sentinel) {
/* Update root. */
parent[(*this).header] = y;
} else {
if (x == left[parent[x]]) {
left[parent[x]] = y;
} else {
right[parent[x]] = y;
}
}

but I don't really like it. Any suggestions for making it look better
and more clear?
May 29 '07 #1
Share this Question
Share on Google+
12 Replies


P: n/a
desktop wrote:
I have made this:

if (parent[x] == &N::sentinel) {
/* Update root. */
parent[(*this).header] = y;
} else {
if (x == left[parent[x]]) {
left[parent[x]] = y;
} else {
right[parent[x]] = y;
}
}

but I don't really like it. Any suggestions for making it look better
and more clear?
It is pretty and clear. No need to change it.
May 29 '07 #2

P: n/a
On Tue, 29 May 2007 17:28:13 +0200 in comp.lang.c++, desktop
<ff*@sss.comwrote,
>but I don't really like it. Any suggestions for making it look better
and more clear?
First step, "chain" the else-if and get rid of the extra level of
brackets.

if (parent[x] == &N::sentinel) {
parent[(*this).header] = y; // Update root.

} else if (x == left[parent[x]]) {
left[parent[x]] = y;

} else {
right[parent[x]] = y;
}

May 29 '07 #3

P: n/a
On May 29, 11:28 am, desktop <f...@sss.comwrote:
I have made this:

if (parent[x] == &N::sentinel) {
/* Update root. */
parent[(*this).header] = y;
} else {
if (x == left[parent[x]]) {
left[parent[x]] = y;
} else {
right[parent[x]] = y;
}
}

but I don't really like it. Any suggestions for making it look better
and more clear?
There's nothing wrong with this. It's got the
K&R style of brackets, and my personal bias is
to line brackets up vertically. So I do it so:

if(parent[x] == &N::sentinel)
{
// if stuff here
}
else
{
// else case stuff here
}

And so on. But this is just a personal bias.
It's not something I'd suggest changing for
existing code that works.

Unless you have some mods to make, I'd suggest
leaving it alone. Working code shouldn't be
touched for "elegance" reasons. If it's doing
what it is supposed to do, and satisfying the
requirements of performance etc., don't risk
adding bugs.

Remember, every bug in your code was put there
by you.

If you have changes to make, where you go depends
on what you need to do. For example, if you were
to need to add a lot of new logic cases, you might
consider something like a table driven system.
Or if the processing for each case got much more
complicated you might consider putting in a function
to do the processing and keep the logic decisions
as simple and clear as reasonably possible. Or you
might consider that you need a state engine of some
kind, and maybe a class to handle it.
Socks

May 29 '07 #4

P: n/a
"David Harmon" <so****@netcom.comwrote in message
news:46**************@news.west.earthlink.net...
On Tue, 29 May 2007 17:28:13 +0200 in comp.lang.c++, desktop
<ff*@sss.comwrote,
>>but I don't really like it. Any suggestions for making it look better
and more clear?

First step, "chain" the else-if and get rid of the extra level of
brackets.

if (parent[x] == &N::sentinel) {
parent[(*this).header] = y; // Update root.

} else if (x == left[parent[x]]) {
left[parent[x]] = y;

} else {
right[parent[x]] = y;
}
This is one instance I would even get rid of the backetrs entirely, although
opionions vary on this.

if (parent[x] == &N::sentinel)
parent[(*this).header] = y; // Update root.
else if (x == left[parent[x]])
left[parent[x]] = y;
else
right[parent[x]] = y;
May 30 '07 #5

P: n/a
On May 30, 9:29 am, "Jim Langston" <tazmas...@rocketmail.comwrote:
"David Harmon" <sou...@netcom.comwrote in message
news:46**************@news.west.earthlink.net...
On Tue, 29 May 2007 17:28:13 +0200 in comp.lang.c++, desktop
<f...@sss.comwrote,
>but I don't really like it. Any suggestions for making it look better
and more clear?
First step, "chain" the else-if and get rid of the extra level of
brackets.
if (parent[x] == &N::sentinel) {
parent[(*this).header] = y; // Update root.
} else if (x == left[parent[x]]) {
left[parent[x]] = y;
} else {
right[parent[x]] = y;
}
This is one instance I would even get rid of the backetrs
entirely, although opionions vary on this.
if (parent[x] == &N::sentinel)
parent[(*this).header] = y; // Update root.
else if (x == left[parent[x]])
left[parent[x]] = y;
else
right[parent[x]] = y;
I think it depends somewhat on the bracketing style. Something
like:

if (parent[x] == &N::sentinel)
{
parent[(*this).header] = y; // Update root.
}
else if (x == left[parent[x]])
{
left[parent[x]] = y;
}
else
{
right[parent[x]] = y;
}

is pretty heavy. The post you were responding to, however,
treats the brackets more or less as part of the spelling of the
keywords, or at least as part of the syntax of if/else. Much
the way Modula or Ada does (except that they aren't brackets in
Modula or Ada).

My own preference is for the first style, but I'll use whatever
the company which pays me says to use. If the company style
normally puts both the opening and closing brackets on a line by
themselves, I have no compunction about dropping them if there
is only a single controlled statement. If it doesn't, and uses
something like the first example, I won't drop them, ever,
because the fact that they aren't there is far less visible.

--
James Kanze (GABI Software) email:ja*********@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34

May 30 '07 #6

P: n/a
Jim Langston wrote:
"David Harmon" <so****@netcom.comwrote in message
news:46**************@news.west.earthlink.net...
>On Tue, 29 May 2007 17:28:13 +0200 in comp.lang.c++, desktop
<ff*@sss.comwrote,
>>but I don't really like it. Any suggestions for making it look
better and more clear?

First step, "chain" the else-if and get rid of the extra level of
brackets.

if (parent[x] == &N::sentinel) {
parent[(*this).header] = y; // Update root.

} else if (x == left[parent[x]]) {
left[parent[x]] = y;

} else {
right[parent[x]] = y;
}

This is one instance I would even get rid of the backetrs entirely,
although opionions vary on this.

if (parent[x] == &N::sentinel)
parent[(*this).header] = y; // Update root.
else if (x == left[parent[x]])
left[parent[x]] = y;
else
right[parent[x]] = y;
(parent[x] == &N::sentinel ?
parent[this->header]
: x == left[parent[x]] ? left[parent[x]]
: right[parent[x]] ) = y;

:-)

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask
May 30 '07 #7

P: n/a
Victor Bazarov wrote:
Jim Langston wrote:
>"David Harmon" <so****@netcom.comwrote in message
news:46**************@news.west.earthlink.net.. .
>>On Tue, 29 May 2007 17:28:13 +0200 in comp.lang.c++, desktop
<ff*@sss.comwrote,
but I don't really like it. Any suggestions for making it look
better and more clear?
First step, "chain" the else-if and get rid of the extra level of
brackets.

if (parent[x] == &N::sentinel) {
parent[(*this).header] = y; // Update root.

} else if (x == left[parent[x]]) {
left[parent[x]] = y;

} else {
right[parent[x]] = y;
}
This is one instance I would even get rid of the backetrs entirely,
although opionions vary on this.

if (parent[x] == &N::sentinel)
parent[(*this).header] = y; // Update root.
else if (x == left[parent[x]])
left[parent[x]] = y;
else
right[parent[x]] = y;

(parent[x] == &N::sentinel ?
parent[this->header]
: x == left[parent[x]] ? left[parent[x]]
: right[parent[x]] ) = y;

:-)
why not
(parent[x] == &N::sentinel && (parent[(*this).header] = y, true)) || (x
== left[parent[x]] && (left[parent[x]] = y, true)) || (right[parent[x]]
= y, true);

:)

Regards,

Zeppe
May 30 '07 #8

P: n/a
desktop wrote:
I have made this:

if (parent[x] == &N::sentinel) {
/* Update root. */
parent[(*this).header] = y;
} else {
if (x == left[parent[x]]) {
left[parent[x]] = y;
} else {
right[parent[x]] = y;
}
}

but I don't really like it. Any suggestions for making it look better
and more clear?
This is the best you can do in C++ but I should note that you have manually
compiled a pattern match. You would get much better results (shorter,
faster code) if you wrote the pattern match in its original form and let a
compiler do the work for you, creating all of the nested ifs.

http://www.ffconsultancy.com/ocaml/b..._matching.html

--
Dr Jon D Harrop, Flying Frog Consultancy
OCaml for Scientists
http://www.ffconsultancy.com/product...ntists/?usenet
May 30 '07 #9

P: n/a
"James Kanze" <ja*********@gmail.comwrote in message
news:11*********************@m36g2000hse.googlegro ups.com...
On May 30, 9:29 am, "Jim Langston" <tazmas...@rocketmail.comwrote:
"David Harmon" <sou...@netcom.comwrote in message
news:46**************@news.west.earthlink.net...
On Tue, 29 May 2007 17:28:13 +0200 in comp.lang.c++, desktop
<f...@sss.comwrote,
>but I don't really like it. Any suggestions for making it look better
and more clear?
First step, "chain" the else-if and get rid of the extra level of
brackets.
if (parent[x] == &N::sentinel) {
parent[(*this).header] = y; // Update root.
} else if (x == left[parent[x]]) {
left[parent[x]] = y;
} else {
right[parent[x]] = y;
}
This is one instance I would even get rid of the backetrs
entirely, although opionions vary on this.
if (parent[x] == &N::sentinel)
parent[(*this).header] = y; // Update root.
else if (x == left[parent[x]])
left[parent[x]] = y;
else
right[parent[x]] = y;
I think it depends somewhat on the bracketing style. Something
like:

if (parent[x] == &N::sentinel)
{
parent[(*this).header] = y; // Update root.
}
else if (x == left[parent[x]])
{
left[parent[x]] = y;
}
else
{
right[parent[x]] = y;
}

is pretty heavy. The post you were responding to, however,
treats the brackets more or less as part of the spelling of the
keywords, or at least as part of the syntax of if/else. Much
the way Modula or Ada does (except that they aren't brackets in
Modula or Ada).

My own preference is for the first style, but I'll use whatever
the company which pays me says to use. If the company style
normally puts both the opening and closing brackets on a line by
themselves, I have no compunction about dropping them if there
is only a single controlled statement. If it doesn't, and uses
something like the first example, I won't drop them, ever,
because the fact that they aren't there is far less visible.

------------------

Ahh, you caught me. Yes, I use a bracket on a line by itself asy our
example showed, which is why I sometimes drop them for single statements.
May 31 '07 #10

P: n/a
On May 30, 2:17 pm, "Victor Bazarov" <v.Abaza...@comAcast.netwrote:
Jim Langston wrote:
"David Harmon" <sou...@netcom.comwrote in message
news:46**************@news.west.earthlink.net...
On Tue, 29 May 2007 17:28:13 +0200 in comp.lang.c++, desktop
<f...@sss.comwrote,
but I don't really like it. Any suggestions for making it look
better and more clear?
First step, "chain" the else-if and get rid of the extra level of
brackets.
if (parent[x] == &N::sentinel) {
parent[(*this).header] = y; // Update root.
} else if (x == left[parent[x]]) {
left[parent[x]] = y;
} else {
right[parent[x]] = y;
}
This is one instance I would even get rid of the backetrs entirely,
although opionions vary on this.
if (parent[x] == &N::sentinel)
parent[(*this).header] = y; // Update root.
else if (x == left[parent[x]])
left[parent[x]] = y;
else
right[parent[x]] = y;
(parent[x] == &N::sentinel ?
parent[this->header]
: x == left[parent[x]] ? left[parent[x]]
: right[parent[x]] ) = y;
:-)
Why the smiley? It's actually not bad, although I'd reformat
slightly:
( parent[x] == &N::sentinel
? parent[this->header]
: x == left[parent[x]]
? left[parent[x]]
: right[parent[x]] )
= y ;
The one thing that would make me hesitate is that I'm not sure
that many people are familiar with the way ?: chains (even
though it is exactly like if/else). To tell the truth, I'd
never really realized it myself until John Potter pointed it out
(and suggested that the above could be a "standard" idiom).

I also feel a little unconfortable using ?: for lvalues. But
that's probably just because I'm not used to it. But it makes
it especially clear that the goal is to assign y to something,
and that we always, unconditionally, update *something* with y.

--
James Kanze (GABI Software) email:ja*********@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34

May 31 '07 #11

P: n/a
On May 30, 3:04 pm, Zeppe
<zep_p@.remove.all.this.long.comment.yahoo.itwrote :
Victor Bazarov wrote:
Jim Langston wrote:
"David Harmon" <sou...@netcom.comwrote in message
news:46**************@news.west.earthlink.net.. .
On Tue, 29 May 2007 17:28:13 +0200 in comp.lang.c++, desktop
<f...@sss.comwrote,
but I don't really like it. Any suggestions for making it look
better and more clear?
First step, "chain" the else-if and get rid of the extra level of
brackets.
> if (parent[x] == &N::sentinel) {
parent[(*this).header] = y; // Update root.
} else if (x == left[parent[x]]) {
left[parent[x]] = y;
} else {
right[parent[x]] = y;
}
This is one instance I would even get rid of the backetrs entirely,
although opionions vary on this.
if (parent[x] == &N::sentinel)
parent[(*this).header] = y; // Update root.
else if (x == left[parent[x]])
left[parent[x]] = y;
else
right[parent[x]] = y;
(parent[x] == &N::sentinel ?
parent[this->header]
: x == left[parent[x]] ? left[parent[x]]
: right[parent[x]] ) = y;
:-)
why not
(parent[x] == &N::sentinel && (parent[(*this).header] = y, true)) || (x
== left[parent[x]] && (left[parent[x]] = y, true)) || (right[parent[x]]
= y, true);
Because unlike Victor's example, that doesn't express the
intent. The intent is to set a particular element to y. Which
element varies, according to the structure of the hierarchy, but
that's a secondary detail. So we put it in a sub-expression of
the main expression, which is "<something= y". The key to
Victor's expression is that the "= y" is an unconditional part
of the top expression, and you don't have to check three
different branches to verify that they all end in an "= y".

--
James Kanze (GABI Software) email:ja*********@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34

May 31 '07 #12

P: n/a
James Kanze wrote:
On May 30, 3:04 pm, Zeppe
>why not
(parent[x] == &N::sentinel && (parent[(*this).header] = y, true)) || (x
== left[parent[x]] && (left[parent[x]] = y, true)) || (right[parent[x]]
= y, true);

Because unlike Victor's example, that doesn't express the
intent. The intent is to set a particular element to y. Which
element varies, according to the structure of the hierarchy, but
that's a secondary detail. So we put it in a sub-expression of
the main expression, which is "<something= y". The key to
Victor's expression is that the "= y" is an unconditional part
of the top expression, and you don't have to check three
different branches to verify that they all end in an "= y".
That's true. It was actually a joke to point out that the Victor's
example was too tricky to understand not having any need to be so (as it
was his intent, given the smiley). I actually would leave the
conditional operator for really simple tests or for some situations that
are difficult to solve otherwise (for example, a test in a copy
constructor).

I'm not afraid of the complex instructions, just I think that they
shouldn't be used if there is no need. And, in that case, that
expression in my opinion doesn't replicate the way the human brain reasons.

Regards,

Zeppe
May 31 '07 #13

This discussion thread is closed

Replies have been disabled for this discussion.