On Wed, Jul 2, 2014, at 01:50 PM, Liam White wrote:

 

I was thinking that should be flagged as it is actually an abuse of the condition.

 

You have two things collapsed to a single line, and at the least it will interfere with debugging. On the other hand, when broken apart, consider the break-point possibilities:

 

1 SPItem *item = dynamic_cast<SPItem*>(obj);

2 if (item) {

3     ...

n }

 

You can now set a breakpoint on line one or on line two. It might seem less useful, but if one combines a condition on a breakpoint at line 2 you can stop only if it is null or only if it not null, etc. However you do lose a bit of safety by leaving that pointer around to possibly use after it is no longer valid. Fixing that gives

 

{

    SPItem *item = dynamic_cast<SPItem*>(obj);

    if (item) {

        ...

    }

}

 

Extra parenthesis can be added at any time to limit scope of any variables. Quite handy, that. However a more logical restructuring could also be. There are other options too, including for loops

 

for (SPItem *item = dynamic_cast<SPItem*>(obj); item; item = NULL) {

}

 

That would address the assignment issue, and keep it to a single line. I'll have to ponder if that is better enough. It is definitely not to the level of abusing the 'for' statement that the for(EVER) idiom is. :-)

 

 

 

But the bigger question would be why use such code to begin with? It is a very C style approach. "Let's just store everything as void* and cast back as needed on the fly". A more C++ centric approach would not have a raw obj pointer, but something more specific. I'd suggest looking at the structure of the surrounding code to see if that can be fixed here and the dynamic_cast<> be avoided completely.