[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Spacewalk-devel] [PATCH] - 672652 Kickstart "Kernel Options" and "Post Kernel Options" reject valid options



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)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]