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'm new to Java and am trying to solve a scenario. Even though I've succeeded and it works, I've just wondered if there was a more practical way of doing this task. Is there a quicker or more efficient way of solving the task, or is this fine?

Task:

Given a string, return a version without the first 2 chars. Except keep the first char if it is 'a' and keep the second char if it is 'b'. The string may be any length.

deFront("Hello") → "llo"
deFront("java") → "va"
deFront("away") → "aay"

Code:

public String deFront(String str) {    
  if(str.length() > 0) {
    if(str.substring(0,1).equals("a") && !str.substring(1,2).equals("b")) {
      return str.substring(0,1) + str.substring(2,str.length());
    } else if(str.substring(1,2).equals("b") && !str.substring(0,1).equals("a")) {
      return str.substring(1,2) + str.substring(2,str.length());
    } else if(str.substring(0,1).equals("a") && str.substring(1,2).equals("b")) {
      return str;
    } else {
      return str.substring(2,str.length());
    }
  } else {
    return "";
  }
}
share|improve this question
    
Worth noting, there's charAt(int) for the single char checks, which expresses your intent far clearer than substring. –  TC1 Dec 11 '13 at 1:20
add comment

4 Answers

up vote 11 down vote accepted

Yes, some of the things you are doing is redundant and quite messy. As this seems to be a homework assignment I will only provide hints for you here.

What you can do is:

  • If the string is empty, return an empty string immediately
  • Store the first character in a variable
  • Store the second character in a variable, if it exists
  • Store the rest of the string in a variable, if it exists
  • Check if the second character is 'b', if it is then add it to the beginning of the string
  • Check if the first character is 'a', if it is then add it to the beginning of the string
  • Return the string

A problem of your existing code is that it will fail for strings of length 1. So it is important to only use the first and second characters if they exist at all.

share|improve this answer
add comment

First of all, thumbs up for solving this problem in a function rather than in main(). I would declare this as a static function, though, since it is a pure function that does not rely on any instance variables in an object.

I see two main problems:

  1. If str has fewer than two characters, you'll get an IndexOutOfBoundsException.
  2. Your strategy doesn't "scale" well, since you are attempting to enumerate all possible combinations of how the string can begin. If you also took into consideration the possibility of str having fewer than two characters, you would end up with a combinatorial explosion.

I recommend a different approach, which mimics the problem description more closely. As you analyze str, build the string that you want to return. For example, if the string length exceeds 0 and the first character is 'a', then append 'a' to your result.

Instead of extracting substrings of length 1, I suggest fetching a character by calling string.charAt(index).

share|improve this answer
add comment

There are a few things I noticed right off of the bat:

  • You never check to see if your input is null. This opens you up immediately to a NullPointerException if your API is misused.
  • You don't handle the case of a 1-length String being passed in, which opens you up to an IndexOutOfBoundsException.
  • You are constantly calling .substring(1, 2) or the like in order to retrieve one character of the String; this is less efficient than using the .chatAt(1) method, and you could simply store the character in a char variable for future reference.
  • There's no need to use something like str.substring(2, str.length()); you can simply call this as str.substring(2). The String API is actually pretty powerful.

Here's my five minute rendition of it. Actually, my version uses inline if statements because I find that it makes the code more concise and readable, but I laid it out a bit more to make it more intuitive for you.

public static String deFront(String str) {

    if(str == null || (str.length() < 2) {
        return "";
    }

    boolean keepFirst = str.charAt(0) == 'a';
    boolean keepSecond = str.charAt(0) == 'b';

    if(keepFirst && keepSecond) {
        return str;
    }
    else if(!keepFirst && !keepSecond) {
        return str.substring(2);
    }
    else if(!keepFirst && keepSecond) {
        return str.substring(1);
    }
    else if(keepFirst && !keepSecond) {
        return str.charAt(0) + str.substring(2);
    }

    throw new IllegalArgumentException("Something weird happened.");    
}

Notice that I immediately check for null in the first line. Because logical operators "short-circuit" in Java, if str == null, the str.length() < 2 condition will never even be evaluated since the || will evaluate to true regardless.

Next, notice that I store whether or not to trim the associated characters in a boolean variable right off the bat. This prevents me from constantly having to examine and break up the passed in str, which not only makes my code slightly more efficient but, more importantly, makes it vastly more readable.

Finally, the throwing Exceptions bit at the end might be a bit more advanced than your level, but there's nothing like a taste of the fun stuff you'll get to do to keep you interested. :) At first I simply had the last line as return null, since every ending branch must return something. But because the code should never reach that line anyway (since I have an exhaustive list of all possibilities in my if/else-if blocks), I thought it was better to throw an Exception (i.e., raise an error the program) in case it ever happened for some reason, since it means something fundamentally broke.

Note that I could have also just left the last else if() block as an else in order to avoid the need to do something like that, but I wanted to make the code as explicit as possible for you to read through and understand.

For reference, here's how I originally wrote it:

public static String deFront(String str) {

    if(str == null || str.length() < 2) return "";

    boolean keepFirst = str.charAt(0) == 'a';
    boolean keepSecond = str.charAt(0) == 'b';

    if(!keepFirst && !keepSecond) return str.substring(2);
    else if(!keepFirst && keepSecond) return str.substring(1);
    else if(keepFirst && !keepSecond) return str.charAt(0) + str.substring(2);
    else return str;
}

One last note: I could have just had them all be if statements as well, since each only returns and removes the method call from the stack anyway, but I find that it makes the code more readable to have else ifs, since it means the conditionals are logically joined to the reader.

EDIT: I actually had to expand the initial checks slightly. Technically speaking, given your rules, if the String is passed in as "a", it should return "a", not the empty string.

public static String deFront(String str) {

    if(str == null) return "";
    else if(str.equals("a")) return str;
    else if(str.length() < 2) return "";

    boolean keepFirst = str.charAt(0) == 'a';
    boolean keepSecond = str.charAt(0) == 'b';

    if(!keepFirst && !keepSecond) return str.substring(2);
    else if(!keepFirst && keepSecond) return str.substring(1);
    else if(keepFirst && !keepSecond) return str.charAt(0) + str.substring(2);
    else return str;
}
share|improve this answer
1  
I don't think it's the API's job to check whether the string is null, rather this looks like something that could hide nasty errors. –  heinrich5991 Dec 10 '13 at 21:50
add comment

I would have solved it using a regular expression with a replace command. It might not be faster than a well written if-tree, but why write the if-tree yourself when the regex engine will do it for you?

(?:(a)|[^a])

For instance matches any one character, but only captures the character if it is a. Build the rest of that regex and you have a clean one-liner solution.

Do remember the edge cases, as others have pointed out you forgot to handle the 1 character case in your code, and it is of course also something you need to think about with this method.

share|improve this answer
    
Although a nice suggestion and something I didn't think about originally, writing a good regex is hard, probably especially for the OP who is "new to Java". –  Simon André Forsberg Dec 12 '13 at 11:49
    
@SimonAndréForsberg It is something to put on the to-learn list, knowing that markedly different methods exist is the first step towards utilizing them. –  eBusiness Dec 13 '13 at 19:03
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.