Sayli, Some comments about your commit to ProfileHandler.java.
http://git.fedorahosted.org/git/?p=spacewalk.git;a=commitdiff;h=d3a6f6fc02e152f9f02d22e273d96dba0af16d80 I don't see the need of using a Set for validOptions. I'd just use a List, that will make the code much more simplified. String[] validOptionNames = new String[] {"autostep", ..., "zfcp"}; List<String> validOptions = Arrays.asList(validOptionNames); ----------------------- While looping and checking at the same time avoids looping through the options List entirely, it makes it a bit harder to read. Instead of doing both the Set building AND the validation, I'd change the code from this: Set<String> givenOptions = new HashSet<String>(); for (Map option : options) { String name = (String) option.get("name"); givenOptions.add(name); if (!validOptions.contains(name)) { throw new FaultException(-5, "invalidKickstartCommandName", "Invalid kickstart command Option: " + name); } } to this: Set<String> givenOptions = new HashSet<String>(); for (Map option : options) { givenOptions.add((String) option.get("name")); } // see if we have all valid options if (!validOptions.containsAll(givenOptions)) { throw new FaultException(-5, "invalidKickstartCommandName", "Invalid kickstart command Option: " + name); } ----------------------- Instead of looping through the requiredOptionNames to see if the givenOptions contain all of the requiredOptions, change the requiredOptions code to the following: List<KickstartCommandName> requiredOptions = ... List<String> requiredOptionNames = new ArrayList<String>(); for (KickstartCommandName kcn : requiredOptions) { requiredOptionNames.add(kcn.getName()); } if (!givenOptions.containsAll(requiredOptionNames)) { throw new FaultException(...); } Sure you loose the EXACT invalid option. I'd just add the validOptions strings to the FaultException and let the caller figure out what's needed to fix the call. ----------------------- Change the "s" variable to be something like "cmds", single variable names are ok for loops or utility methods, but looking at s I have no idea what it is. ----------------------- Get rid of the index variable. Instead of this: int index; KickstartCommandName cn = (KickstartCommandName) itr.next(); index = -99; if (givenOptions.contains(cn.getName())) { for (int i = 0; i < options.size(); i++) { if (cn.getName().equals(options.get(i).get("name"))) { index = i; } } Try this: Map option = null; KickstartCommandName cn = (KickstartCommandName) itr.next(); if (givenOptions.contains(cn.getName())) { for (Map o : options) { if (cn.getName().equals(o.get("name"))) { option = o; break; } } // now replace options.get(index).get("arguments") simply with // option.get("arguments") ... cmds.add(kc); } Thanks, jesus _______________________________________________ Spacewalk-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/spacewalk-devel
