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 if
s, 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;
}
charAt(int)
for the single char checks, which expresses your intent far clearer thansubstring
. \$\endgroup\$