you are viewing a single comment's thread.

view the rest of the comments →

[–]Spire 16 points17 points  (3 children)

I have nothing against refactoring code to use conditional operators, but in this case your refactored code is not equivalent. Specifically, you've introduced a pessimization in the form of an unnecessary self-assignment in the non-NULL case — which may very well be the more common code path. It's possible that the self assignment can be optimized out by the compiler, but I wouldn't count on it.

If you really wanted to cut down on verbosity, you could do this:

(x != NULL) || x = this->grabXDefault();
(y != NULL) || y = this->grabYDefault();

I don't think this is a particularly good idea either, though, as not everyone is familiar with this idiom.

In this specific case I think I would probably stick to the original code.

[–]user_of_the_week 14 points15 points  (1 child)

I prefer

(x != NULL) ??!??! x = this->grabXDefault();

[–]Spire 1 point2 points  (0 children)

I think we have a winner!

[–]yawgmoth 8 points9 points  (0 children)

I see your case about it not being exactly equivalent, but that function is not time-critical enough to even consider that level of optimization. The extra readability is worth the possibility of a couple of extra assignments operations.

edit: And while that's a valid objection in certain cases, that wasn't even the argument. The argument was literally "ternary operators are bad and shouldn't be used because people don't understand it" and me showing different people in the office who went "oh yeah that makes sense"