[rfc][icedtea-web] Option Parser

[Jie Kang]

----- Original Message -----
> Hello,
> 
> I have taken in as much of your suggestions as I could. I have attached the
> updated Option Parser. I rewrote how the parser works and changed it from
> parsing inside a method to parsing in the constructor, as you should only
> really need to parse once. It stores the options in a Map with Options as
> Keys and a List of Strings for the values. The getValue(s) methods return
> based on the option from that map.
> 
> 
> >> +    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?
> 
> In Boot it gets used by being put in a Map<String, String[]> which is then
> passed into the Launcher class.
> I didn't want to change that up in this patch too. If you want I can add
> another method that could be used in the future called getListOfValues.
> 
> >> +    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.
> 
> I looked into what was using this and it was ParserSettings so I redid the
> method to accept two booleans instead of
> args which it would then look for the "-strict" and "-xml" tags inside of.
> This also meant changing the ParserSettingsTest which in my opinion don't
> make sense in their current state. I plan on looking into Parser and
> ParserSetting in the future and see how they are used and if it makes sense
> for them to be changed to incorporate the OptionParser.
> 
> The OptionsDefinitions Class is a shared class Jiri made that I have helped a
> bit on, I will update my optionsDefinitions Class with any further changes
> if required by Jiri. Also will update with the version Jiri sends out to the
> list.
> 
> For the parser I tried my best to make it as flexible as possible to cover as
> many possible various cases of how users might input their commands.
> 
> For example, the user can use the same tag multiple times and it won't matter
> 
> command -headless -headless -headless File.jnlp -headless -headless -headless
> 
> is the same as
> 
> command -headless File.jnlp
> 
> also for ones that take multiple inputs you can have the tag multiple times
> 
> command -arg blue green red -headless File.jnlp -arg yellow purple
> 
> will have blue, green, red, yellow, purple all under arg
> Notice however that -headless need to be before File.jnlp since the parser
> wouldn't know whether File.jnlp was just another arg or the main argument
> but after -headless which it knows takes no arguments it can be sure that
> File.jnlp is the main arg.
> 
> Currently writing a few more unit tests and testing out other cases but
> wanted to get feedback before I spent too much time on more unit tests.
> 
> Thank you,
> Lukasz Dracz


Hello,


Review in-line with code:

+public class OptionParser {
+
+    private String[] args;
+    private Map<OptionsDefinitions.OPTIONS, List<String>> parsedOptions;

Please make both args and parsedOptions final as well

+    private final List<OptionsDefinitions.OPTIONS> possibleOptions;
+    private final String MAINARG = "mainArg";
+    private final OptionsDefinitions.OPTIONS mainArg = null;

Not a big fan of using the null key. Please add a comment to clarify that it is meant to be null and what it represents.

+    private final boolean lookForMainArg;
+
+    public OptionParser(String[] args, List<OptionsDefinitions.OPTIONS> options, boolean lookForMainArg) {
+        this.args = Arrays.copyOf(args, args.length);
+        this.possibleOptions = options;
+        this.lookForMainArg = lookForMainArg;
+        List<String> result = new ArrayList<String>();
+        String lastOption = "";
+        parsedOptions = new HashMap<>();

Moving the parsing of arguments into the constructor is fine but please extract it into it's own private function or possibly their own functions and have them called in the constructor.

+        int i = 0;
+        while (i < args.length) {
+            if (isOption(args[i])) {
+                result.clear();
+                if (args[i].contains("=")) {
+                    result.add(args[i].split("=")[1]);
+                    lastOption = args[i].split("=")[0];
+                    parsedOptions.put(getOption(lastOption), new ArrayList<String>(result));
+                } else {
+                    lastOption = args[i];
+                    if (parsedOptions.keySet().contains(getOption(lastOption))) {
+                        if (getOption(lastOption).hasManyArguments()) {
+                            result = new ArrayList<>(parsedOptions.get(getOption(lastOption)));
+                        }
+                    }
+                    if (getOption(lastOption).hasNoArguments()) {
+                        parsedOptions.put(getOption(lastOption), null);
+                        lastOption = MAINARG;
+                    }
+                }
+            } else {
+                result.add(args[i]);
+                parsedOptions.put(getOption(lastOption), new ArrayList<String>(result));
+                if (getOption(lastOption) != null) {
+                    if (getOption(lastOption).hasOneArgument()) {
+                        lastOption = MAINARG;
+                        result.clear();
+                    }
+                }
+            }
+            i++;
+        }
+
+        if(this.lookForMainArg) {
+            if (!(parsedOptions.keySet().contains(mainArg))) {
+                i--;

This is a style nit but having this 'i--' in here requires more code-reading than say 'i = args.length - 1'. With the latter it's more quickly understood that you are iterating args in reverse. Up to you;

+                while (i >= 0) {
+                    if (!args[i].startsWith("-")) {
+                        for (OptionsDefinitions.OPTIONS op : parsedOptions.keySet()) {
+                            if (!(parsedOptions.get(op) == null)) {
+                                if (parsedOptions.get(op).contains(args[i])) {
+                                    result.clear();
+                                    result.add(args[i]);
+                                    parsedOptions.get(op).remove(parsedOptions.get(op).indexOf(args[i]));
+                                    break;
+                                }
+                            }
+                        }
+                        parsedOptions.put(mainArg, result);
+                        break;
+                    }
+                    i--;
+                }
+            }
+        }
+
+    }


+        public String getLocalizedEscription() {
typo?;
+            return R(decriptionKey);
+        }



Regards,

-- 

Jie Kang