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.
--
vda