2
\$\begingroup\$

I am coding the following interesting problem :

Remove '_' and print with 1 space from a given string For example, for "w_e_b__services" output should be : "w e b services". Multiple underscore should be substituted with only with one space.

The following is my code, which has some code duplication. I can see there are some ways to get rid of that duplication but that complicates the code in a different way. It would be great to get some code review on this.

public class ReplaceUnderscore {

    private static final char UNDERSCORE = '_';
    private static final char SPACE = ' ';

    public static String replace(String input) {

        if (input.isEmpty()) {
            return input;
        }

        StringBuilder output = new StringBuilder();

        char prevChar = input.charAt(0);
        if (prevChar == UNDERSCORE) {
            output.append(SPACE);
        } else {
            output.append(prevChar);
        }

        for (int i = 1; i < input.length(); i++) {
            char currentChar = input.charAt(i);

            if (currentChar == UNDERSCORE) {
                if (currentChar != prevChar) {
                    output.append(SPACE);
                }
            } else {
                output.append(currentChar);
            }

            prevChar = currentChar;
        }

        return output.toString();
    }
}

And here is the junit test:

public class StripUnderscoreTest {

    @Test
    public void strip() throws Exception {

        assertEquals(" ", ReplaceUnderscore.replace("_"));
        assertEquals("a b c", ReplaceUnderscore.replace("a_b_c"));
        assertEquals("a b c", ReplaceUnderscore.replace("a__b__c"));
        assertEquals("a b c", ReplaceUnderscore.replace("a___b___c"));
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ @yuri Nope regex is not an option for me \$\endgroup\$
    – Exploring
    Mar 6, 2018 at 22:27
  • \$\begingroup\$ I would suspect that a Finite State Machine is "overkill" for this kind of problem, but I would highly recommend you look into Finite State Machines for future more complicated versions of this kind of problem. \$\endgroup\$
    – Dair
    Mar 7, 2018 at 1:24
  • 6
    \$\begingroup\$ Just as a remark on regex not being an option: yes, this might be OK for educational purposes. But in real life (TM) there is no reason at all for a self-made implementation which replaces the readily available replaceAll in String. Thus, from a professional I'd expect the solution to be s.replaceAll("_+", " ") and no more. \$\endgroup\$
    – mtj
    Mar 7, 2018 at 6:39

3 Answers 3

2
\$\begingroup\$

I think overall it is a nice implementation. It is clear and readeable. The use of constants is nice, makes it easy to refactor the code to make it more generic later on (e.g. replace with given character, not just space). Clear variable names (although I'd try and be consistent, and use current\previous, or cur/prev, most likely the former).

I have two remarks:

  • I am not a fan of the new line at the start of a function, it is too much, and makes me lose my focus.
  • I'd change the check if (currentChar != prevChar) to if (prevChar != UNDERSCORE). Although we can know from the check above that currentChar is an underscore, this makes it even more clear.

I don't really think you have what I'd see as duplication, but a possible other way to implement it would be to keep track of wether you are currently in a sequence of underscores. Example:

boolean previousWasUnderscore = false;
for (int i = 0; i < input.length(); i++) {
    final char currentChar = input.charAt(i);
    if (currentChar == UNDERSCORE) {
        if (!previousWasUnderscore ) {
            output.append(SPACE);
            previousWasUnderscore = true;
        }
    } else {
        output.append(currentChar);
        previousWasUnderscore = false;
    }
}
\$\endgroup\$
0
1
\$\begingroup\$

For generality, I would define a function that accepts the old and new delimiters as parameters, then overload it to make the underscore and space as defaults.

To avoid the special case for input.isEmpty(), I would define a boolean variable to indicate whether the previously encountered character was an underscore. You would also avoid having to re-inspect the previous character to see whether it was an underscore.

You can edit the string in place (using the same buffer for the input and output). It's slightly less cumbersome than calling the StringBuilder methods, in my opinion.

public class ReplaceUnderscore {

    public static String replace(String input) {
        return replace('_', ' ', input);
    }

    public static String replace(char oldDelim, char newDelim, String input) {
        boolean wasOldDelim = false;
        int o = 0;
        char[] buf = input.toCharArray();
        for (int i = 0; i < buf.length; i++) {
           assert(o <= i);
           if (buf[i] == oldDelim) {
               if (wasOldDelim) { continue; }
               wasOldDelim = true;
               buf[o++] = newDelim;
           } else {
               wasOldDelim = false;
               buf[o++] = buf[i];
           }
        }
        return new String(buf, 0, o);
    }
}
\$\endgroup\$
1
\$\begingroup\$

You may also find this solution iteresting using an inner empty loop to skip the consecutive underscores. This works because in java assignments like (currentChar = input.charAt(++i)) also have a return value (the assigned value). I also use the pre increment ++i to get the next char index.

for (int i = 0; i < input.length(); i++) {
    final char currentChar = input.charAt(i);
    if (currentChar == UNDERSCORE) {
        while(++i < input.length() && (currentChar = input.charAt(i))==UNDERSCORE)
           ; // skip undercores with empty loop
        output.append(SPACE);
    }
    output.append(currentChar);
}

alternatively you can use a foot controlled while:

for (int i = 0; i < input.length(); i++) {
    final char currentChar = input.charAt(i);
    if (currentChar == UNDERSCORE) {
        do { 
           currentChar = input.charAt(Math.min(++i, input.length()-1))
        while(i < input.length() && currentChar == UNDERSCORE); 
        output.append(SPACE);
    }
    output.append(currentChar);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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