To be fair, I think that the given block could be written better both in terms of clarity and in terms of knocking out unneeded functions.
s = fd == -1 ? socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP) : fd;
if (s == -1)
goto bad;
Is the equivalent of
if (fd == -1)
{
s = -1;
goto bad;
}
else
{
s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP);
}
but with one less comparison, assignment of a constant to s in the error condition instead of a variable, which is usually faster, and a more clear, explicit, layout. Now, the compiler may make these optimizations for you, but it still leaves an uglier bit of code. Tristate operators can be useful, but in some cases, like this, they can make the code less efficient.
Your refactor is wrong. You've reversed the ternary and removed the socket() return check. The consolation prize is that you've reinforced the issue of lack of clarity in the code.
You're right. I was thrown off by them putting the exceptional condition as the first part of the ternary operation. Chalk this to overly clever code.
if (fd != -1)
{
s = fd;
}
else
{
s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP);
if (s == -1)
{
goto bad;
}
}
or in rewriting the ternary function for clarity:
// If we have a socket, use it, otherwise, try to get one
s = (fd != -1) ? fd : socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP);
if ( s == -1 ) // we still don't have a socket, error out
goto bad;
Ternary operators should, in my opinion, be put to the 3 am test. If you think you'd get confused at looking at code that uses one at 3 am, then you've not written it properly and should clarify.
