current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 6 down vote favorite

I have the situation where as long as the user gives valid input, the StringBuilder object will not be null.

StringBuilder sb = null;
switch(userInput)
{
  case 1:
    sb = method1();
    break;
case 2:
    sb = method2();
    break;
case 3:
    sb = method3();
    break;
case 4:
    sb = method4();
    break;
default:
    System.out.println("invalid input"); 
}

System.out.println(sb.toString);//gives warning "dereferncing possible null pointer

Update: for those commenting it should be String instead of StringBuilder could you please explain? The method does a lot of appending so it needs to use String. Why would you bother converting to String if you can directly print out a StringBuilder? It seems like there's an implicit belief that a StringBuilder should always be converted to a String.

share|improve this question

closed as off-topic by 200_success♦ May 20 '14 at 19:44

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.
    
When you say : The method does a lot of appending do you refer to method1 or actually the code your showing? I can get that method1 does a lot of appending and could need an StringBuilder, but in the code your showing I don't see any String manipulation only a sb.toString (which should be sb.toString()) –  Marc-Andre May 20 '14 at 19:40
3  
Generic identifiers method1 … method4 make this hypothetical code, which is off-topic for Code Review. Please ask a follow-up question with real or realistic details. –  200_success♦ May 20 '14 at 19:47
    
@Marc-Andre I mean inside method1...method4 does a lot of appending. –  Celeritas May 20 '14 at 20:47

2 Answers 2

up vote 11 down vote accepted

You need to ensure that every statement in the switch is one that ensures that sb != null holds, this is not the case currently.

You can fix this by throwing an exception instead of logging a message, like this:

default:
    throw new IllegalArgumentException("input was invalid: " + userInput);

I also included that it tells you what the actual input was.

Other remarks are that your code has syntax errors, making it borderline ontopic, and your coding style definately needs improvement, I propose the following hence:

StringBuilder sb = null;
switch (userInput) {
    case 1:
        sb = method1();
        break;
    case 2:
        sb = method2();
        break;
    case 3:
        sb = method3();
        break;
    case 4:
        sb = method4();
        break;
    default:
        throw new IllegalArgumentException("input was invalid: " + userInput);
}

System.out.println(sb.toString());

Furthermore it seems very odd to me that your methods return a StringBuilder. I would just work with Strings here, as you are not doing an extensive amount of string concatenation.

Short guide:

  • String, to be used for regular text.
  • StringBuilder, to be used when you need to concentate/append a lot of strings.
  • StringBuffer, similar to StringBuilder, but now add to that that you are working in a multithreaded environment, hence you will almost never need this.
share|improve this answer
    
but StringBuilder is necessary inside method. Technically you're just wasting time converting it to a String to return. Can you explain your view some more? –  Celeritas May 20 '14 at 19:25
1  
@Celeritas How are you wasting time exactly? There are two scenarios: 1) You convert it before returning from method1(), which is the idiom that literally everyone is using. Or 2) You convert it after it has been returned, which you are using now. There is wasting time and besides that, it is nothing you should worry about, you should read up on Premature Optimization. –  skiwi May 20 '14 at 19:27
    
that's exactly what I'm trying to get at. Is it some sort of "idiom" to return a String than a StringBuilder? I'm saying it's a waste of time to typecast to String because I could just do System.out.println(myStringBuilder); which is all I need. –  Celeritas May 20 '14 at 20:49
    
There is no "waste of time" since it will happen anyway in println(). What is more important is that your returning a builder when actually what you really need is a String. What is your goal of method1 ? If it's building a String than build it and return it. The caller don't need to know that you're using a StringBuilder to create a String. –  Marc-Andre May 20 '14 at 20:54
up vote 2 down vote

In the default clause of your switch you are not setting sb to a non-null value. So it might still be null after the switch. You could change the default clause to set sb to something non-null, or throw an Exception like @skiwi suggested, which is a very good idea by the way.

Btw, you don't really need the .toString() call on a StringBuilder. This works fine too:

System.out.println(sb);

If its value is null, it will print the text "null".

Finally, I don't know what your complete code looks like, but if you're not using the StringBuilder returned by your method1(), method2(), ..., to build strings, then it might make sense to change those methods to return a simple String instead of a StringBuilder.

share|improve this answer
    
I'm up to hear your argument as to why String over StringBuilder. I guess I made the decision because if the string needed to be changed in the future it may be easier. Also it saves space in the code because instead of converting to String in each method. –  Celeritas May 20 '14 at 19:01
    
Also how does that work you don't need .toString()? Is some sort of implicit type conversion happening? –  Celeritas May 20 '14 at 19:02
1  
@Celeritas StringBuilder is merely a mutable builder to build strings with, it (in itself) is not a String. You should only use it for building strings. "if the string needed to be changed in the future", is not a good argument to use a StringBuilder. The only good argument would be if you need to change the string right now, and then we are talking about 10+ times. –  skiwi May 20 '14 at 19:04
    
@skiwi no it's not like that. Inside the method the string gets changed a whole bunch and string builder is required. What I'm saying is may as well wait to the last possible minute to convert it to a string in case it needs to be changed, instead of returning a string from the method. –  Celeritas May 20 '14 at 19:06
    
@Celeritas The outside code, like sb = method1(), doesn't care about the implementation, it doesn't need to add anything to that string anymore. Hence you should keep using a StringBuilder inside method1(), then build the String in the method and return it. –  skiwi May 20 '14 at 19:08

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