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
