HalosGhost / pbpst

A command-line libcurl C client for pb deployments
GNU General Public License v2.0
44 stars 9 forks source link

Dramatically clean up Option Validation #31

Closed HalosGhost closed 9 years ago

HalosGhost commented 9 years ago

Though what we have “works,” it is about as ugly as code comes (despite having already been cleaned up a bit). We need to set up a simpler, cleaner (and preferably faster) way of validating the use of particular arguments together.

One way to do this would be to forego validation altogether and make multiple calls to getopt_long(). Doing so would solve a lot of these cleanliness issues. But it would bloat our option declaration code a bit along with bloating main() a bit as well since it is hard to move getopt() calls outside of main().

Another option is to continue on the course we are on now, but declare problems with option compatibility in the getopt_long() call itself. This would mean we would fail out faster, and we would trade-off the horribly-ugly validation function with a bit more validation in-line. This would also, likely, be dramatically faster in most cases.

Finally, we could just keep the validation function we have now but work out a way to simplify it so that we only need to check validation in a single call, we bloat nothing else in the code and we keep things simple.

I'm leaning towards the last option for now, but I have am still open to suggestions.

HalosGhost commented 9 years ago

There's another option that seems like it may be the best of all worlds. Instead of having an option validation function or manually setting compatibility in the getopt switch, we do the following:

This avoids nested calls to getopt_long() along with the associated bloat. It also avoids all the extra option-validation function calls and junk we are doing now. It does mean we may (in the worst-case scenario) parse the arguments two times (which would make the worst-case have slightly better performance than nested getopt_long() calls). However, in the best case, it lets us fail out much sooner than our current schema and should generally still be faster than what we're doing now (because of the removal of all the logical branches).

This is now the most interesting option in my opinion.

HalosGhost commented 9 years ago

We've now implemented most of this. There is still a bit of validation junk left over (though it has been dramatically cleaned up), but all it checks now is that no options passed are incompatible with one another. I have not decided how best to clean up those, but the critical mass of horrible has been removed (via ba9867c). I'm considering this closed.