On 08/31/2011 09:21 PM, Tomas Lestach wrote:
Hey Marcelo,
I checked your patch and here're my comments:
* I do not really understand following change:
- if (StringUtils.isEmpty(kernelOpts)) {
+ if (kernelOpts == null || kernelOpts.equals("")) {
According to
http://commons.apache.org/lang/api-2.5/org/apache/commons/lang/StringUtils.html#isEmpty(java.lang.String)
both lines shall do exactly the same job. I'd like to use again the
StringUtils methods.
* When I started to work on Spacewalk, I was told not to use
'instanceof', there's always a better way, how to write the code :-)
+ } else if(map.get(key) instanceof List<?>){
why do you actually use it? Do you expect anything else than a List?
* third line looks redundant here:
+ List<String> list = (List)toRet.get(split[0]);
+ list.add(split[1]);
+ toRet.put(split[0], list);
Also my 3 cents to the conversation:
- for (Object key : map.keySet()) {
- if (StringUtils.isEmpty((String)map.get(key))) {
+ for (String key : map.keySet()) {
+ if (map.get(key) == null) {
string.append(key + " ");
- }
1. This won't handle an empty String as a value of the map anymore
(i.e. not null) as before. I would suggest to revert the code back
to use StringUtils instead, which checks both.
2. If you use StringBuffer, normally you want to use chains instead of
just concatenating pieces of strings, like:
string.append(key).append(" ");
...and this on the rest of the patch.
HTH.
--
Bo Maryniuk
SUSE LINUX Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
HRB 16746 (AG Nürnberg)
_______________________________________________
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel
