std.getopt suggestion

[Jonathan M Davis]

On Thursday, September 29, 2011 00:40:44 Andrei Alexandrescu wrote:
> On 9/28/11 12:44 PM, Jonathan M Davis wrote:
> > Okay. I have a suggestion for an improvement to std.getopt that I think
> > merits a bit of discussion. There's currently a pull request with some
> > improvements for getopt which are mostly internal changes rather than
> > API changes ( https://github.com/D-Programming-Language/phobos/pull/272
> > ), but I think that there is an API change that we should consider.
> > 
> > Right now, there are three configuration options which are mutable
> > module-level variables:
> > 
> > dchar optionChar = '-';
> > string endOfOptions = "--";
> > dchar assignChar = '=';
> > 
> > and the aforementioned pull request adds another for an array separator.
> > Mutable module/global variables are generally considered to be bad
> > design
> > (though they're sometimes necessary), and I'm very much inclined to have
> > those variables _not_ be at the module scope like that.
> 
> Why?

1. Mutable globals are generally considered to be bad practice. As you 
yourself have stated before, Phobos should be an example of good software 
practices in D. Having mutable globals goes directly contrary to that goal.

2. Conceptually, what happens with getopt is that you configure it and then run 
it. It is much better encapsulation for the configuration to be tied to the 
function call. The normal way to do this (at least in an OO language) is to 
make it a member function of the configuration. As it stands, those 
configuration variables are just sitting in the same module. As getopt is 
really the only function of consequence in the module, it's not as big a deal 
as it would be in most modules, but it's still better encapsulation to 
actually tie the configuration to the function that it's configuring.

3. By putting the configuration options in a struct, they are better organized. 
It makes it easier to see what the whole list is without having to search the 
module. It also gives a very good place to document them all together.

4. If you need to run getopt multiple times - particularly if you need to run 
it with different configurations each time - it's definitely cleaner to do that 
when you can just use a different GetOpt instance in each case. You don't have 
to worry about one run of getopt affecting another. Now, this matters far less 
for getopt than it would your average function, since it would be highly 
abnormal to need to run getopt multiple times like that, but the general 
design principle holds.

5. Assuming that we were creating std.getopt from scratch, there would be 
_zero_ benefit in having any of its configuration options be at the module 
level. There is a definite argument for leaving them there given that moving 
them could break code (though honestly, it wouldn't surprise me if no one in 
the history of D has ever written a program that changed any of those 
variables from their defaults given how standard they are and how little gain 
there normally is in changing them), but from the perspective of design, I 
don't see any reason why it would ever be better to have the variables be at 
module scope. On the contrary, it goes against what is generally considered 
good design.

6. Putting the configuration in a struct would probably allow getopt to be pure 
(depending on its implementation). I don't know if there would ever be a 
program where that would really matter given how getopt is normally used, but 
it would be a benefit to having the configuration encapsulated in a struct 
rather than at module scope. And as it stands, getopt definitely can't be pure, 
so if it ever did matter, it would be a problem.

Honestly, I think that it primarily comes down to it being better design to 
encapsulate the configuration options together, tying them to the function that 
they're for, rather than having mutable variables at module scope. And Phobos 
should not only be well-designed on general principle, but it's supposed to be 
an example good D code and practices, and as it stands, std.getopt doesn't do 
that with regards to module-level variables. The only reason that I see to 
leave it as it is is because it's already that way.

And if we mess with what's currently there in order to change any of the 
defaults for the config enum (as opposed to the variables at module scope which 
we've been discussing) and/or to change getopt to getOpt to follow Phobos' 
naming conventions, it just makes sense to change anything about the design 
which is suboptimal which can be reasonably changed.

- Jonathan M Davis