Re: bugs in idvec-verify.c, almost...

[Marcus Brinkmann]

On Sat, Sep 06, 2003 at 03:40:34PM -0400, Roland McGrath wrote:
> > >  if (getpwuid_r (id, &_pw, id_lookup_buf, sizeof id_lookup_buf, &pw)
> > >      == 0)
> > 
> > That code is ugly anyway, we should do function calls outside of if
> > conditions.  So this could be cleaned up along with the semantic fix.
> 
> There is nothing wrong with this kind of conditional.

Well, it's true that there is no particular problem with them, if you mean
potential bugs or something (ie, if compared to assignments in conditions).

But a helper variable and splitting it up in two lines can greatly enhance
readability, because it avoids the need to parse the code horizontically,
and it keeps the line short.  In the following line, there are two
interesting parts.  One is the function call, the other is the check of the
return value.  And the check of the return value is at the very end of the
line.

 if (getpwuid_r (id, &_pw, id_lookup_buf, sizeof id_lookup_buf, &pw) == 0)

In the following, the interesting parts are in two separate lines, and both
are at the beginning of the line.

  int ret;

  ret = getpwuid_r (id, &_pw, id_lookup_buf, sizeof id_lookup_buf, &pw);
  if (ret == 0)

If you are not really interested in the function arguments, but just in the
general flow control (what happens under which conditions?), the latter is
much more readable, IMHO.

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' GNU      http://www.gnu.org    marcus@gnu.org
Marcus Brinkmann              The Hurd http://www.gnu.org/software/hurd/
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de/