[rfc][icedtea-web] Option Parser

[Omair Majid]

Hi,

Comments on patch below:

* Lukasz Dracz <ldracz at redhat.com> [2014-08-25 16:32]:
> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java	Mon Aug 25 16:04:08 2014 -0400

>              + "  -Xoffline             " + R("BXoffline") + "\n"
>              + "  -help                 " + R("BOHelp") + "\n";

The help string now contains a duplicate list of the option name
strings. Perhaps you should consider moving the descriptions along with
the options?

> +        OptionParserConfig optionParserConfig = OptionParserConfig.JAVAWS;
> +        optionParser = new OptionParser(argsIn, optionParserConfig);

Just a nit: the generic variable name `optionParserConfig` is making
this code harder to read. Perhaps inlining the variable will make it
easier to read?

    optionParser = new OptionParser(argsIn, OptionParserConfig.JAVAWS);

> +            List<JavaWSOption> secondLastArg = Arrays.asList(JavaWSOption.BASEDIR, JavaWSOption.ARG, JavaWSOption.PARAM, JavaWSOption.PROPERTY, JavaWSOption.UPDATE);

Is this a list of all options that take an argument? That really should
be part of the option enum itself.

> +            for (JavaWSOption option : secondLastArg) {
> +                if(optionParser.getArgs()[optionParser.getArgsLength()-2].equals(option.toString())) {
> +                    return optionParser.getArgs()[optionParser.getArgsLength()-1];
> +                }
> +            }

Can you simplify this logic? I am having trouble making sense of this.

> +++ b/netx/net/sourceforge/jnlp/util/optionparser/JavaWSOption.java	Mon Aug 25 16:04:08 2014 -0400

> +public enum JavaWSOption implements Option{

> +    BASEDIR("-basedir"),
> +    JNLP("-jnlp"),
> +    ARG("-arg"),
> +    PARAM("-param"),
> +    PROPERTY("-property"),
> +    UPDATE("-update"),
> +    HEADLESS("-headless"),
> +    VIEWER("-viewer"),
> +    VERSION("-version"),
> +    LICENSE("-license"),
> +    HELP("-help"),
> +    ABOUT("-about"),
> +    VERBOSE("-verbose"),
> +    NOUPDATE("-noupdate"),
> +    NOFORK("-Xnofork"),
> +    TRUSTALL("-Xtrustall"),
> +    TRUSTNONE("-Xtrustnone"),
> +    OFFLINE("-Xoffline"),
> +    NOSECURITY("-nosecurity"),
> +    CLEARCACHE("-Xclearcache"),
> +    IGNOREHEADERS("-ignoreheaders"),
> +    ALLOWREDIRECT("-allowredirect");

A couple of suggestions; all trying to make this class contain
all the logic and information needed to make use of the options:

1. Can you leave out the initial '-' from the option name in the
declaration?

2. Please add a flag or so to indicate if an option takes an argument.

3. Please include the help string (maybe something short and maybe
something longer?).

> +++ b/netx/net/sourceforge/jnlp/util/optionparser/Option.java	Mon Aug 25 16:04:08 2014 -0400

> +public interface Option {
> +
> +}

Please use a separate method, like `getName()` for getting the option name.
Leave `toString()` for debugging and pretty printing or something.

> +++ b/netx/net/sourceforge/jnlp/util/optionparser/OptionParser.java	Mon Aug 25 16:04:08 2014 -0400

> +public class OptionParser {

> +    private String[] args;
> +    private OptionParserConfig optionParserConfig;

Please mark these as final. Also consider renaming `optionParserConfig`
to `config`. It's shorter but conveys pretty much the same information.

> +    public OptionParser(String[] args, OptionParserConfig option) {
> +        this.args = Arrays.copyOf(args, args.length);
> +        this.optionParserConfig = option;
> +    }

So there's two schools of thought on protecting against (possibly
malicious) mutation:

- Make a defensive copy of everything

- Make things immutable

I see you used the first approach for `args` but nothing for
`optionParserConfig`. Please defend that too. My suggestion is to make
it all (both the parser and parser config class) immutable. 

> +    public String[] getArgs() {
> +        return Arrays.copyOf(args, args.length);
> +    }

Eh? Why expose internal state? Think about what needs to use this, and
if an alternate implementation that does not expose the internals would
be possible and/or better.

At least, please switch return type from `String[]` to `List<String>`.

> +    private boolean isOption(String s) {
> +        for(Option opt : optionParserConfig.getOptionsList()){
> +            if (opt.toString().equals(s)) {
> +                return true;
> +            }
> +        }
> +        return false;
> +    }

Minor suggestion to improve readability: define this private method
below the method that uses it.

> +     * Return all the values of the specified option, or an empty
> +     * array if the option is not present.  If the option is
> +     * present with no parameters then the option name is
> +     * returned once.

What's the reason behind separating no-optione case from
option-supplied-but-no-value? Can you use a better sentinel for this?
Returning the option name itself seems a little weird.

> +    public String[] getOptionValues(Option option) {
> +        List<String> result = new ArrayList<String>();

> +        return result.toArray(new String[result.size()]);
> +    }

Why not just return a `List<String>` instead?

> +++ b/netx/net/sourceforge/jnlp/util/optionparser/OptionParserConfig.java	Mon Aug 25 16:04:08 2014 -0400

> +public enum OptionParserConfig {
> +
> +    Class theClass;

Please make this private.

> +        if(option.getClass() == theClass)
> +        {
> +            return true;
> +        }

Please fix the parenthesis style.

Thanks,
Omair

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