Denys Vlasenko <dvlasenk(a)redhat.com> writes:
On Tue, 2011-08-02 at 09:34 +0200, Nikola Pajkovsky wrote:
> Denys Vlasenko <dvlasenk(a)redhat.com> writes:
> > The if and loop looks wrong:
>
> Why do you think that it's wrong? That would mean, that man page of
> getopt_long is wrong also :)
It is not wrong as in "buggy", but it's coded in a way
which is less readable than needed.
> > if (optind < argc)
> > {
> > while (optind < argc)
> > {
> > const char *dump_dir = argv[optind++];
> > ...
> > if (optind - argc)
> > printf("\n");
> > }
> > exit(0);
> > }
> >
> > show_usage_and_die(program_usage_string, program_options);
> >
> > return 0;
Let's enumerate the readability problems here.
* Error check for "no args supplied" has a form where
non-error branch - which is bigger that error branch -
comes first, and sits in indented if {...}:
if (args supplied)
{
...
...
...
...
...
exit(0);
}
show_usage_and_die();
Considering that in general the "..." part may be very large
(pages of code), such constructs look much more readable
in this form:
if (!args supplied)
show_usage_and_die();
...
...
...
...
...
exit(0);
Reader doesn't need to figure out "why this code is in this large if()"
- because there is no large if() anymore.
> if (optind - argc)
What does it mean? It means "there are more args".
"if (argv[optind])" is a much more readable form of the same check.
(However, in my version below I decided to also
eliminate double checking (one in while() and one in this if())
and in the course of the conversion this condition got inverted).
> > I'd rewrite it like this:
> >
> >
> > if (!argv[optind])
> > show_usage_and_die(program_usage_string, program_options);
> >
> > while (1)
> > {
> > const char *dump_dir = argv[optind++];
> > ...
> > if (!argv[optind])
> > break;
> > printf("\n");
> > }
> >
> > return 0;
Apart from readability, on efficiency side I got rid of exit(0) and
extra condition check.
I see your point (and it's a good point). I'll invert the logic as you
suggest.
--
Save the whales. Feed the hungry. Free the mallocs