Style: Declaration in if/while-condition - good or bad?

M

Martin Ba

C(++) allows for a simple variable declaration+init in the condition of
an if or while:

while (T t = x) {
}

if (int x = f()) {
}

I find this rather useful for things like:

if (const T* pThing = GetThing()) {
...
} else {
// Handle No-Thing case
}


However, I have been told by a colleague that this is little used and
would surprise and confuse a lot of developers and therefore it's better
to use the form:

const T* pThing = GetThing();
if (pThing) {
...
} else {
// Handle No-Thing case
}


Opinions?

cheers,
Martin
 
V

Victor Bazarov

C(++) allows for a simple variable declaration+init in the condition of
an if or while:

while (T t = x) {
}

if (int x = f()) {
}

I find this rather useful for things like:

if (const T* pThing = GetThing()) {
...
} else {
// Handle No-Thing case
}


However, I have been told by a colleague that this is little used and
would surprise and confuse a lot of developers and therefore it's better
to use the form:

const T* pThing = GetThing();
if (pThing) {
...
} else {
// Handle No-Thing case
}


Opinions?

Your colleague's opinion seems rather unfounded. AFAICT it is quite a
commonplace to have a pointer declared in the 'if' condition to be used
only when it evaluates in non-null expression. Unless there is a need
to use 'pThing' after the 'if' statement, there is no reason for
'pThing' to be declared in the scope outside of the 'if' it has been
evaluated for. Does this help? I hope so.

V
 
A

Alain Ketterlin

Martin Ba said:
while (T t = x) {
}

if (int x = f()) {
}

[instead of]
const T* pThing = GetThing();
if (pThing) {
...
} else {
// Handle No-Thing case
}

They are not equivalent, because the former restricts the scope of the
declaration (you would need a pair of braces around the second piece of
code to get the same effect).

I find myself using this more and more, often changing my code to make
these patterns usable. I think you should not worry about potential
"surprise and confusion", because the code looks to me completely
unambiguous, and therefore it takes almost no time to get used to.

-- Alain.
 
T

Tobias Müller

Martin Ba said:
C(++) allows for a simple variable declaration+init in the condition of an if or while:

while (T t = x) {
}

if (int x = f()) {
}

I find this rather useful for things like:

if (const T* pThing = GetThing()) {
...
} else {
// Handle No-Thing case
}


However, I have been told by a colleague that this is little used and
would surprise and confuse a lot of developers and therefore it's better to use the form:

const T* pThing = GetThing();
if (pThing) {
...
} else {
// Handle No-Thing case
}


Opinions?

Regardless whether someone is actually using it or not, it should at least
be clear what it does. I don't understand how this is confusing. Everyone
uses it with 'for' loops.

Personally, I use it from time to time but it has also some drawbacks.

Pro:
- Keeps the outer scope clean.
- Declaration is very close to the point where it is used, especially in
'else if' conditions.
Con:
- Only possible with implicit conversion to bool instead if explicit
condition (e.g. pThing != NULL).
- Complex conditions are not possible. E.g: if (pThing &&
pThing->IsValid())

Because of that limitations, one can argue that if the condition changes in
the future, you have to move it to the outer scope anyway. And those
changes are potentially dangerous (because you increase the scope of a
variable) and more difficult to review than just changing the condition.

Tobi
 
B

Balog Pal

I find this rather useful for things like:

if (const T* pThing = GetThing()) {
...
} else {
// Handle No-Thing case
}


However, I have been told by a colleague that this is little used and
would surprise and confuse a lot of developers and therefore it's better
to use the form:

const T* pThing = GetThing();
if (pThing) {
...
} else {
// Handle No-Thing case
}

The latter form is way inferior. And developers who actually get
confused by it (after a 5-minute training) are better have their code
checkin right suspended. And a workplace with abundance of such people
is better left ASAP.
 
B

Balog Pal

Con:
- Only possible with implicit conversion to bool instead if explicit
condition (e.g. pThing != NULL).
- Complex conditions are not possible. E.g: if (pThing &&
pThing->IsValid())

Yeah, too bad we have restrictions (other one popping up with for that
we need multiple vars, and only one is supported; I'm generally not
happy to use tuple as workaround).

But that is not an argument against use where actually possible.
Because of that limitations, one can argue that if the condition changes in
the future, you have to move it to the outer scope anyway. And those
changes are potentially dangerous (because you increase the scope of a
variable) and more difficult to review than just changing the condition.

That sounds pretty weird. And as a purely fear-based speculative argument.

Future changes are problems of the future. Covered well by YAGNI, and
was experience on our inability to correctly predict the future. So
better drop all speculation and keep the code simplest and easiest to
read. Now, and ever. For the task that must be done.

And I don't see why review the change later would be more dangerous or
time-consuming than doing it right now.

While leaving variable around will keep an educated reader wondering on
its purpose.
 
S

Stefan Ram

Balog Pal said:
The latter form is way inferior. And developers who actually get
confused by it (after a 5-minute training) are better have their code
checkin right suspended.

If it helps the OP to get a kind of an idea of the
prevailing/general/average opinion: I agree!
 
8

88888 Dihedral

Balog Palæ–¼ 2013å¹´2月28日星期四UTC+8上åˆ3時30分38秒寫é“:
The latter form is way inferior. And developers who actually get

confused by it (after a 5-minute training) are better have their code

checkin right suspended. And a workplace with abundance of such people

is better left ASAP.

How about getting 2 to 5 things in different classes to work together
in a safe way if anything went wrong?
 
M

Martin Ba

That sounds pretty weird. And as a purely fear-based speculative argument.

It seems fear-based arguments are "common" when discussing C++. Maybe
because it's such an awe-inspiring language :)
 
M

Martin Ba

I think the declaration in the condition style is fine for simple and
clear cases (and minimises the scope of the variable, which is always
good practice) - but don't do it for complex assignments as they can
easily be unclear.

Note that some C/C++ checkers will flag "if (x = GetThing()) ..." with a
warning (note the lack of declaration - this assumes "x" is already
defined and in scope) since it looks very much like a typo with "="
instead of "==". gcc will flag this when you have a lot of warnings
enabled - use "if ((x = GetThing()))" with extra parenthesis to avoid
the warning.

There is also a VC++ warning for this and you can't prevent it with any
parenthesis. (C4706 assignment within conditional expression)

The warning is actually the reason why I got into this argument in the
first place, because I said we can prevent the warning by declaring the
var inside the if instead of before.

cheers,
Martin
 
A

army1987

There is also a VC++ warning for this and you can't prevent it with any
parenthesis. (C4706 assignment within conditional expression)

`if ((x = GetThing()) != 0)`?
 
Ö

Öö Tiib

On Thu, 28 Feb 2013 11:23:18 +0100, Martin Ba wrote:

`if ((x = GetThing()) != 0)`?

Your "fix" removed declaration from inside of if that is required to be exactly there by OP. ;)
 
J

James Kanze

C(++) allows for a simple variable declaration+init in the condition of
an if or while:
while (T t = x) {
}
if (int x = f()) {
}
I find this rather useful for things like:
if (const T* pThing = GetThing()) {
...
} else {
// Handle No-Thing case
}
However, I have been told by a colleague that this is little
used and would surprise and confuse a lot of developers and
therefore it's better to use the form:
const T* pThing = GetThing();
if (pThing) {
...
} else {
// Handle No-Thing case
}
Opinions?

I wouldn't say little used, because a lot of C++ programmers
seem to like obfuscation (and it was intentionally added to the
standard, so someone must like it). Still, such compactation
does nothing to improve readability, and when I see it in code
I have to work on, the first thing I'll do is to remove it.
 
J

James Kanze

[...]
while (T t = x) {
}
if (int x = f()) {
}
[instead of]
const T* pThing = GetThing();
if (pThing) {
...
} else {
// Handle No-Thing case
}
They are not equivalent, because the former restricts the scope of the
declaration (you would need a pair of braces around the second piece of
code to get the same effect).

If that makes a difference, your functions are too large. And
note that in the case of `if`...`else`, the variable _is_ in
scope in the `else` branch.
I find myself using this more and more, often changing my code
to make these patterns usable. I think you should not worry
about potential "surprise and confusion", because the code
looks to me completely unambiguous, and therefore it takes
almost no time to get used to.

Anyone trying to understand your code has a right to suppose
that `if` means `if`, and not `if`, plus define a new variable.
You can write all of your functions in a single line, too, but
that doesn't help readability.
 
J

James Kanze

On 28/02/2013 10:23, Martin Ba wrote:

There are two things here - it looks a bit odd to put "if
(type var = ...) into code. But the fact that it limits the
scope of the variable is a far more powerful argument than it
relying on the implicit conversion to bool.

The implicit conversion to `bool` is, of course, a killer
argument. This is one of the most confusing features of C/C++;
and even after more than 30 years of using the language, I still
get confused by things like `if ( strcmp(...) )`. Even in C, it
was more less recognized good practice to program as if the
language had a boolean type, the comparison operators returned
a boolean type, and there were no implicit conversions to
`bool`. Those have been the rules in every coding guidelines
I've seen in those 30 years.

As for limiting scope: if this is a significant issue, you have
a larger problem: your functions are too long.
 
S

Stefan Ram

James Kanze said:
Anyone trying to understand your code has a right to suppose
that `if` means `if`

This sounds strange from someone to whome the grave accent

<`> 96, Hex 60, Octal 140 GRAVE ACCENT, SPACING GRAVE

means »quotation mark«.

»If« means »if«, but not »if«!

That is, we are using C++, not English.

»If« in C++, means the C++ »if«, not the English »if«.

In C++, we read and write code like »if( int const c = ...«,
in English we do not.
 
B

BCFD36

I wouldn't say little used, because a lot of C++ programmers
seem to like obfuscation (and it was intentionally added to the
standard, so someone must like it). Still, such compactation
does nothing to improve readability, and when I see it in code
I have to work on, the first thing I'll do is to remove it.

I have read the whole thread. There is some food for thought, but in
general I like the 2nd method. Reasons:
1. Limiting the scope is usually not THAT important. If you have
multiple items with the same thing in the same routine, just in
different scopes, your code is going to be very confusing.
2. When debugging, you can breakpoint on the line with the call.
Depending on the debugger, you may not easily be able to step into
GetThing(). Stepping out also could get interesting.
3. As a matter of taste, I don't like doing multiple things on the same
line, let alone in the same statement. Things can get really muddy.
4. I make exceptions for for loops. Declaring the iterator in the loop
statement is pretty clear. Doing one or more function calls within an if
statement just muddies things up. And the "=" vs "==" can shoot you
right in the foot. And my feet have enough holes in them, thank you very
much.
 
M

Martin Ba

The implicit conversion to `bool` is, of course, a killer
argument. This is one of the most confusing features of C/C++;
and even after more than 30 years of using the language, I still
get confused by things like `if ( strcmp(...) )`. Even in C, it
was more less recognized good practice to program as if the
language had a boolean type, the comparison operators returned
a boolean type, and there were no implicit conversions to
`bool`. Those have been the rules in every coding guidelines
I've seen in those 30 years.

Care to give any references? (How many have you seen? By whom?)

T* p;
....
if (p) {
}

seems perfectly fine to me.
As for limiting scope: if this is a significant issue, you have
a larger problem: your functions are too long.

I see it merely as a minor (vs. significant) issue, but an issue it
still is, even in short functions.


cheers,
Martin
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top