views:

1436

answers:

10

In C++ you can initialize a variable in an if statement, like so:

if (CThing* pThing = GetThing())
{
}

Why would one consider this bad or good style? What are the benefits and disadvantages?

Personally i like this style because it limits the scope of the pThing variable, so it can never be used accidentally when it is NULL. However, i don't like that you can't do this:

if (CThing* pThing = GetThing() && pThing->IsReallySomeThing())
{
}

If there's a way to make the above work, please post. But if that's just not possible, i'd still like to know why.

Question borrowed from here, similar topic but PHP.

+2  A: 

This shoulddoesn't work in C++ sinceeven though it supports short circuiting evaluation. MaybeDon't try the following:

if ((CThing* pThing = GetThing()) && (pThing->IsReallySomeThing()))
{
}

err.. see Wesley Tarle's answer

Luke
I'll try ... i remember i tried all kinds of bracket placement but none would work, although the compiler errors changed a lot depending on the brackets.
steffenj
"short circuiting evaluation" ... i have known this only as "early out" so far. This one's new to me.
steffenj
+2  A: 

One reason I don't normally do that is because of the common bug from a missed '=' in a conditional test. I use lint with the error/warnings set to catch those. It will then yell about all assignments inside conditionals.

Steve Fallows
+1  A: 

About the advantages:

It's always recommended to define variables when you first need them, not a line before. This is for improved readability of your code, since one can tell what CThing is without scrolling and searching where it was defined.

Also reducing scope to a loop/if block, causes the variable to be unreferenced after the execution of the code block, which makes it a candidate for Garbage Collection (if the language supports this feature).

Pablo Fernandez
+2  A: 

Just an FYI some of the older Microsoft C++ compliers(Visual Studios 6, and .NET 2003 I think) don't quite follow the scoping rule in some instances.

for(int i = 0; i > 20; i++) {
     // some code
}

cout << i << endl;

I should be out of scope, but that was/is valid code. I believe it was played off as a feature, but in my opinion it's just non compliance. Not adhering to the standards is bad. Just as a web developer about IE and Firefox.

Can someone with VS check and see if that's still valid?

J.J.
It's a compiler option, "enforce scope in for statement" or something.
Martin Beckett
Yep, it's /Zc:forScope- "force conformance in for loop scope". At least in VS 2008.
DK
Thanks for checking guys. I am guessing it's on(breaking scope) by default?
J.J.
A: 

You can also enclose the assignment in an extra set of ( ) to prevent the warning message.

Martin Beckett
A: 

I see that as kind of dangerous. The code below is much safer and the enclosing braces will still limit the scope of pThing in the way you want.

I'm assuming GetThing() sometimes returns NULL which is why I put that funny clause in the if() statement. It prevents IsReallySomething() being called on a NULL pointer.

{
    CThing *pThing = GetThing();
    if(pThing ? pThing->IsReallySomeThing() : false)
    {
    // Do whatever
    }
}
Adam Pierce
Derek Park
James Hopkin
+4  A: 

The important thing is that a declaration in C++ is not an expression.

bool a = (CThing* pThing = GetThing()); // not legit!!

You can't do both a declaration and boolean logic in an if statement, C++ language spec specifically allows either an expression or a declaration.

if(A *a = new A)
{
    // this is legit and a is scoped here
}

How can we know whether a is defined between one term and another in an expression?

if((A *a = new A) && a->test())
{
    // was a really declared before a->test?
}

Bite the bullet and use an internal if. The scope rules are useful and your logic is explicit:

if (CThing* pThing = GetThing())
{
    if(pThing->IsReallySomeThing())
    {
    }
}
Wesley Tarle
Richard Corden
I'll accept this answer for the technical verbosity. The discussion about which is better is indeed highly subjective, i prefer the style from my initial post, but my TD doesn't want me to use it, so i don't. I can always use an extra pair of brackets to limit scope as a compromise.
steffenj
A: 

also notice that if you're writing C++ code you want to make the compiler warning about "=" in a conditional statement (that isn't part of a declaration) an error.

Wesley Tarle
A: 

It's acceptable and good coding practice. However, people who don't come from a low-level coding background would probably disagree.

ilitirit
A: 
if (CThing* pThing = GetThing())

It is bad style, because inside the if you are not providing a boolean expression. You are providing a CThing*.

CThing* pThing = GetThing();
if (pThing != NULL)

This is good style.

Daniel Daranas