Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have some doubts on code review tools violation

1) Most of the warnings in any code checking tools we get final keyword could be used for any String used in a method. Is this really required unless u have strong reason, sorry if this question sounds stupid but i am want to know

2) Avoid if (x != y) ..; else ..;

if( !Utils.isEmpty( someobjec) ){
        if( LOGGER.isDebug() ){
            LOGGER.debug( "somedebug " );
        }
    }
    else{
        someobjec) = method
    }

How else could this be written?

3) A method should have one exit point

Example:

public static String doSomething( String str ){
    if( Utils.isEmpty( str ) ){
        return "";
    }
    if( str.length() == 1 ){
        return "&#" + str.toCharArray()[ 0 ] + ";";
    }
           if( str.length() == 3 ){
        return "&#" + str.toCharArray()[ 0 ] + ";";
    }

    String result="something";
            return result;
}

4) The StringBuffer constructor is initialized with size 47, but has at least 400 characters appended violation.

 StringBuffer jspContent = new StringBuffer("somegarbage");
 jspContent.append("somegarbagesomegarbage");
share|improve this question
1  
Use StringBuilder((int)maxSizeNeeded + 16) instead of StringBuffer (obsolete). It more useful to create a StringBuilderwith the maximum expected, and working with free space for other cases than increase internal size by small steps. –  cl-r Nov 23 '12 at 14:30

4 Answers 4

1) You should definitely be using final, see: Does use of final keyword in Java improve the performance?

2) Many other people have suggested how this can be resolved (flipping the if/else statements)

3) Again, this has already been sufficiently answered by others.

4) You should be using StringBuilder, not StringBuffer.

share|improve this answer

3) A method should have one exit point

That is a leftover from C. And just plain stupid. Disable it.

1) final keyword could be used

While this is actually useful, asserting some property of your code that could automatically be checked by the compiler, go ahead and disable it, if you feel it is too much a hassle to write and too messy to read. Parameters can almost always be declared final, and parameters should have been final by default, (since it is much less restrictive than C++ const).

Actually go ahead and disable (convince your team to disable) all the controversial rules and anything you think is unnecessary or isn't worth the effort after reading the rule's rationale. Otherwise, you have reams and reams of warnings that no one pays attention to; soon enough developers start to think static analysis is useless.

share|improve this answer

Some answers (from my point of view):

  1. Switch of the warning (nearly all tools are configurable). This is not a strict error but has some advantages to declare such variables as final. In my view this is a little bit of if you want it or not.
  2. Rewrite the code as
    if (Utils.isEmpty(someobjec)) {
        someobjec.method();
    } else {
        if (LOGGER.isDebug()) {
            LOGGER.debug("somedebug");
        }
    }
    
    most times it is easier to handle positive expressions than negative ones (that's what the message is about).
  3. It is easier to manage a method with only one return (exit point). Most times it is very difficult to track a method that has more than one return statement. Your example could be rewritten as follows:
    public static String doSomehing(String str) {
        String result = "something";
        if (Utils.isEmpty(str)) {
            result = "";
        } else if (str.length() == 1) {
            result = new StringBuilder("&#").append(str.toCharArray()[0]).append(";").toString();
        } else if (str.length() == 3) {
            result = new StringBuilder("&#").append(str.toCharArray()[0]).append(";").toString();
        }
        return result;
    }
    
    I also used the StringBuilder to concat the string parts because this is the way it should be used. So I see no reason to have more than one return statement.

As I stated this is my personal point of view. Other developers may see this different therefore tools as PMD offer possibilities to configure the warnings.

share|improve this answer
    
for the final keyword i agree that it is my call i can decide wthere i can do it or not but for 2nd and 3rd i have doubt as to how else i can implement it specially for only one exit point lets take an example of this method public static String doSOmehing( String str ){ if( Utils.isEmpty( str ) ){ return ""; } if( str.length() == 1 ){ return "&#" + str.toCharArray()[ 0 ] + ";"; } if( str.length() == 3 ){ return "&#" + str.toCharArray()[ 0 ] + ";"; } return result.toString(); } –  VIckyb Nov 23 '12 at 12:06
    
Can you please add the code to your question? In comments the format is lost. Also where is the variable result coming from? –  Uwe Plonus Nov 23 '12 at 12:08
    
I have edited my post with code –  VIckyb Nov 23 '12 at 12:16
    
ONe more violation that i found was StringBuffer constructor is initialized with size 47, but has at least 400 characters appended. an example –  VIckyb Nov 23 '12 at 12:39
    
You can initialize the the of the StringBuffer with a size, if you know it before. But I used StringBuilder for creating the string, because it is not synchronized. –  Uwe Plonus Nov 23 '12 at 12:43

For point 3, you could avoid that if-else and replace it with a switch case.

public static String doSOmehing(String str) {
  String result = "something";

  switch(str.length()) {
  case 0:
    result = null;
    break;

  case 1:
    result = "&#" + str.toCharArray()[ 0 ] + ";";
    break;

  case 3:
    result = "&#" + str.toCharArray()[ 0 ] + ";";
  }

  return result;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.