[rfc][icedtea-web] Option Parser

[Omair Majid]

Hi,

This is great!

* Lukasz Dracz <ldracz at redhat.com> [2014-08-22 10:45]:
> I refactored the way Boot parses arguments by creating an Option
> Parser to use.

Just to be clear, this patch changes behaviour too, right? If so, it's
not completely fair to call it a refactor. From reading the patch the
following invocation is treated differently before and after:

    javaws -arg -update /path/to/some.jnlp

> The reason is that this code could be reused in other
> parts, instead of writing duplicate code. I have also added unit tests
> for the parser.

There's a bit of a problem here. Before this patch, one class knew
everything about options (what options are possible, what their help
messages are, what options take an argument) but now two classes have
this information spread out.

Have you thought about renaming the OptionParser class to Options or
something and moving more information about options in there? Or maybe
OptionParser should not know anything about options and a third class
that creates OptionParser should know all about options (and other
option-related information) and tell OptionParser what it needs to use?

> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java	Fri Aug 22 10:22:15 2014 -0400

>      private static final String doubleArgs = "-basedir -jnlp -arg -param -property -update";

Is this variable used any more?

> +        argsParser = new OptionParser(argsIn);

Code like this is a hint that maybe you should have called your class
ArgumentParser :)

> -        if (null != getOption("-headless")) {
> +        if (argsParser.checkOption(Option.HEADLESS)) {

Would it be easier to read this code if checkOption was renamed to
hasOption? I can't quite figure out (just by looking at the method name)
what check it is performing.

> +++ b/netx/net/sourceforge/jnlp/util/OptionParser.java	Fri Aug 22 10:22:15 2014 -0400

> +//Parses options (generally from main(String[] args)
> +public class OptionParser {

I am not a fan of comments, but in cases like this, it is probably a
good idea to comment on the style of options it parses (--style or
-style). Maybe also add a note for developer on how to add support for
more options?

> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/OptionParserTest.java	Fri Aug 22 10:22:15 2014 -0400

Thanks for writing these awesome unit tests!

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681