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 came across this snippet and am wondering if I should return blank in this case.

Which is more practical to return: blank or null?

public String getInfo() {
    String result = "";
    String uri = clientConfiguration.getUri();
    String regex = "http://(.*)/consolidate";
    Pattern pattern = Pattern.compile(regex);
    Matcher matcher = pattern.matcher(uri);
    if (matcher.find()) {
        result = matcher.group(1);
    }
    return result;
}

Edit: Should I change it to:

 if(matcher.find()) { 
   return matcher.group(1); 
 } 
 return null;
share|improve this question
7  
It is entirely dependent on the context (ie, callers); what is more, your regex is seriously flawed. Hint: use the URI class to parse URIs in general (and this includes URLs) –  fge Feb 21 at 10:33
    
The question is too generic in its current form to be answered as a code review; it may be appropriate for Programmers SE. Could you provide more information about what you are trying to accomplish, and what the code that will be calling this function will do with the result? –  200_success Feb 21 at 10:42
    
Sorry my question is not so clear. Actually I would like to rewrite the last 4 lines as this: if(matcher.find()) { return matcher.group(1); } return null; The thing I wonder is Should I? –  Truong Ha Feb 21 at 10:45
add comment

6 Answers 6

The general question you ask "Which is better, "" or null?" is off-topic for CodeReview, but your code snippet has a number of reviewable items....

Regex Usage

Compiled Pattern performance

  String regex = "http://(.*)/consolidate";
  Pattern pattern = Pattern.compile(regex);
  Matcher matcher = pattern.matcher(uri);
  if(matcher.find()){
      result = matcher.group(1);
  }

This snippet makes it look like you know what you are doing, with the Pattern compile, etc. But, there is no performance benefit in the way you have done this. Compiled Java Pattern instances are thread-safe, and compiling them for one-time-use is not useful. It is common practice to make the Pattern a static-final field:

private static final Pattern MYPATTERN = Pattern.compile("http://(.*)/consolidate");

Then you can re-use that compiled pattern as much as you like, in any method, in any thread, like:

  Matcher matcher = MYPATTERN.matcher(uri);
  if(matcher.find()){
      result = matcher.group(1);
  }

find vs. matches

Now, matcher.find() and matcher.matches() are different methods.

  • find() will scan the input looking for any point inside the input where the pattern will match....
  • matches() does just one scan, and it matches the entire input string against the entire pattern.

With the input junkhttp://a/consolidate/junk:

  • find() will find your pattern
  • matches() will not find your pattern

The pattern

Now, as for the actual Regex... it appears that you want your pattern to match the HTTP 'host' against which you have the 'consolidate' path.... but, your pattern will match a lot of things which I would consider to be unexpected... for example, your pattern will return the following:

http://myhost/consolidate   from  http://http://myhost/consolidate/consolidate
myhost/consolidate          from  http://myhost/consolidate/consolidate
myhost:8080                 from  http://myhost:8080/consolidate
                            from  http:///consolidate

Each of the above input values will produce unexpected results.

Correct Regex

There is not a correct regex for matching URL's.... even if a regex appears that it will match, it is still not the correct solution ;-)

Solution

Use the java.net.URI class to validate your input. I have an example here, where I choose to return null if there is no configured URI, or throw an exception if there is a configured URI and that URI is not a valid value. This would be 'sensible' for many configurations, I expect.

public String getInfo() {
      String urival = clientConfiguration.getUri();
      if (urival == null || urival.isEmpty()) {
          return null;
      }
      try {
          URI uri = new URI(urival);
          return uri.getHost();
      } catch (URISyntaxException e) {
          throw new IllegalStateException("The configured URI is not valid: " + e.getMessage(), e);
      }
}
share|improve this answer
    
"There is not a correct regex for matching URL's..." I'm sure there is one somewhere out there which gets very close, but it most likely looks similar to the E-Mail address verification regular expression. –  Bobby Feb 21 at 13:51
    
@Bobby indeed, if the url language is regular then there will be a regex that accepts it –  ratchet freak Feb 21 at 15:21
    
Just to note: That doesn't mean that I think you should us regular expressions for that...you shouldn't...seriously... –  Bobby Feb 21 at 15:35
add comment

I agree with fge, it depends on the context. A third approach: it might be better to throw an exception in that case.

Another note: you don't need to declare the result variable at the beginning of the method, declare on the first use. Actually, you could completely get rid of it:

    if (matcher.find()) {
        return matcher.group(1);
    }
    return "";

(See also: Effective Java, Second Edition, Item 45: Minimize the scope of local variables It has a good overview on the topic.)

share|improve this answer
    
I would compress it even further, with return matcher.find()? matcher.group(1):"";. –  AJMansfield Feb 21 at 16:39
add comment

As "getInfo" does not hint at something functional or even crucial - only displaying the info, its usage should not fail with a NullPointerException or need to catch a thrown not-found exception. So neither return null here, nor throw an IllegalStateException, but return an empty string.

Better however would be to return an informative string "(Info missing)".

Should the result of getInfo() being used instead of only displayed, throw an exception. In this way the usage can be skipped. This already might be the case, if you embed the result is a surrounding text message. A comparable strategy is done for the hideous NumberFormatException.

Returning null, like in JDBC for field values, requires discipline by the user, and makes only sense in a context where more such "optional" data can be retrieved (= the null does not come unsuspected).

For good order: In Java 8 there also is the class Optional to indicate the optional presence of an object, and getting the object in a next step. (Overkill here)

share|improve this answer
add comment

Really, does every return value need to be assigned to a value? The code would be far clearer if you removed these spurious variables. result should be removed as well; just use two return statements. And for such a small piece of code, I would also suggest shortening matcher, both to avoid confusion with Matcher, and to allow further statements to be more concise.

/** 
 * You need to add javadoc!
 * @return The info, or an empty string if there is no info.
 */
public String getInfo() {
    Matcher m = Pattern.compile("http://(.*)/consolidate")
            .matcher(clientConfiguration.getUri());

    return m.find()?m.group(1):"";
}

Some people insist one must use a full if statement, regardless of the occasion. In any case, don't use a result variable if returns will do, like this:

if(m.find()) return m.group(1);
return "";

As for returning "" or null, as long as you document it in your javadoc (with an @return), either way is fine.

share|improve this answer
    
+1 overall, but 1-liner if(m.find()) return m.group(1); means you can't put a debug breakpoint on the return. –  rolfl Feb 21 at 17:01
    
@rolfl although you can put a breakpoint on the whole thing, and then step through it from there, if needed, or just stick a line break in. –  AJMansfield Feb 21 at 17:05
add comment

If your question is whether you could refactor the code to return null instead of "", without knowing more about your code, I would say no.

Changing the return value from "" to null may break existing code calling your method, and expects it to always return a string value.

share|improve this answer
add comment

It is always better to return null, in place of blank if you are not getting any info. Your method name goes to getInfo(). Returning blank means you get some information but it is blank. But returning null means you did not get any information at all.

It is all upon you to decide what exactly you want to deduce from the returned value.

return null;
share|improve this answer
add comment

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.