11
\$\begingroup\$

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 "";
  }
}
\$\endgroup\$
1
  • \$\begingroup\$ Worth noting, there's charAt(int) for the single char checks, which expresses your intent far clearer than substring. \$\endgroup\$
    – TC1
    Dec 11, 2013 at 1:20

8 Answers 8

17
\$\begingroup\$

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.

\$\endgroup\$
0
7
\$\begingroup\$

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).

\$\endgroup\$
7
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ Dec 10, 2013 at 21:50
  • \$\begingroup\$ I interpret the question to be: keep the second character if the second character is b. \$\endgroup\$ Apr 27, 2014 at 0:54
3
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ 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". \$\endgroup\$ Dec 12, 2013 at 11:49
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ Dec 13, 2013 at 19:03
2
\$\begingroup\$

You may do the following as well:

  • Store the string without the first two characters in a variable say str1.

  • Check if the second character is 'b', if it is then add it to the beginning of the string str1.

  • Check if the first character is 'a', if it is then add it to the beginning of the string str1.

    String str = "away";
    String str1 = str.substring(2);
    
    if(str.substring(1).startsWith("b"))
        str1 = "b" + str1;
    if(str.startsWith("a"))
        str1 = "a" + str1;
    
\$\endgroup\$
1
  • 1
    \$\begingroup\$ This is mostly a duplicate of Simon's answer, and you don't handle strings of length 0 or 1 - these would fail with IndexOutOfBoundsException. It's a good attempt at an implementation, though. \$\endgroup\$
    – Pimgd
    Feb 19, 2016 at 12:45
2
\$\begingroup\$

I think this problem shouldn't be solved by using the substring method. The reason why is that substring does to much things and is way too complicated.

When you take a look to your code you can see the effect of this. The thing that annoys me the most is that you need to call substring multiple times.

Even @asteri solution uses at least 3 substring calls.

So I implemented a solution with a StringBuilder

public String dePrefix(string prefix, string value){
    StringBuilder builder = new StringBuilder();
    for(int i = 0; i < value.length; ++i){
        if(i < prefix.length && value.charAt(i) != prefix.charAt(i)){
            continue;
        }
        builder.append(value.charAt(i));
    }
    return builder.toString();
}

public static String deFront(String str) {
    return dePrefix("ab", str);
}
\$\endgroup\$
1
\$\begingroup\$

You could build your final string using ternary operators and avoid all the ifs. I think this is much more readable:

String firstChar = str.charAt(0) == 'a' ? "a" : "";
String secondChar = str.charAt(1) == 'b' ? "b" : "";
String remainingChars = str.substring(2, str.length());
String finalString = firstChar + secondChar + remainingChars;
\$\endgroup\$
1
  • \$\begingroup\$ Readability is important. What is the result for str = "a"? \$\endgroup\$
    – greybeard
    Jan 19, 2020 at 14:52
-1
\$\begingroup\$

This seems to work:

public String deFront(String str) {
  return (str.startsWith("a") ? "a" : "") + (str.matches(".b.*") ? "b" : "") + (str.length() > 2 ? str.substring(2) : "");
}

Or even shorter with regex replaceAll:

public String deFront(String str) {
  return str.replaceAll("^((a)|[^a])((b)|[^b])?(.*)", "$2$4$5");
}
\$\endgroup\$
1
  • 4
    \$\begingroup\$ Hi, and welcome to Code Review. Offering a suggested alternative to the original problem is a valuable contribution, but only if you add in why the alternative is better, or what problems it solves in the original code. Please consider updating your answer. \$\endgroup\$
    – rolfl
    Apr 27, 2014 at 1:50

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