On Wed, Jul 2, 2014, at 01:50 PM, Liam White wrote:
So if I write
if (SPItem* item = dynamic_cast<SPItem*>(obj)) { // do something with item }
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.
-- Jon A. Cruz jon@...18...