Refactoring String manipulating method with TDD

Question

I'd like to refactor my extractSimplePromptName test so it does the same but it's "prettier", hence less code. As far as looking at it, it seems like it could be refactored so it does the same thing, but it's just less code. I'm just fairly new to programming so having a hard time seeing where I should make the changes.

Guessing it has something to do with putting some stuff out of the first if sentence or perhaps just removing something that doesn't do anything.

I just ain't that certain, and I'd really like some help so I can get better at doing decent code.

@Test
public void makeSimpleName1() {
    String promptNameOrg = "20129142\\1234";

    String simplePromptName = extractSimplePromptName(promptNameOrg);
    Assert.assertEquals("1234", simplePromptName);
}

@Test
public void makeSimpleName2() {
    String promptNameOrg = "80808080\\159;20129142\\1234";

    String simplePromptName = extractSimplePromptName(promptNameOrg);
    Assert.assertEquals("1234", simplePromptName);
}

@Test
public void makeSimpleName3() {
    String promptNameOrg = "159;1234";

    String simplePromptName = extractSimplePromptName(promptNameOrg);
    Assert.assertEquals("1234", simplePromptName);
}

private String extractSimplePromptName(String promptNameOrg) {
    String simplePromptName = "";
    if (promptNameOrg != null) {
        if (promptNameOrg.contains(";") || promptNameOrg.contains("\\")) {
            List<String> splitPrompts = Arrays.asList(promptNameOrg.split(";"));

            String promptName = splitPrompts.get(splitPrompts.size() - 1);
            if (promptName.contains("\\")) {
                String[] split = promptName.split("\\\\");
                List<String> splitPromptName = Arrays.asList(split);
                simplePromptName = splitPromptName.get(splitPromptName.size() - 1);
            } else {
                simplePromptName = promptName;
            }

        } else {
            simplePromptName = promptNameOrg;
        }

    }
    return simplePromptName;
}

Answer 1

Often, when performing text manipulation, Regular Expressions can do the work for you. Your solution is essentially manually building a text parser and stripper, when a regular expression will do the work in a much more concise way.

The regular expression would be to remove all text up to the last \ or ; character, which would be written as:

private String extractSimplePromptName(String promptNameOrg) {
    return promptNameOrg == null ? "" : promptNameOrg.replaceAll("^.*(\\\\|;)", "");
}

Answer 2

Your tests are named horribly; make sure they convey what is being tested.

---

Your code makes it look as if you're testing some helper method inside your tests. What exactly is going on? Make sure that you are actually testing your application and aren't extracting all the effort into a "helper" method in your tests.

That being said I'll assume that your extractSimplePromptName method is actually the application. If it isn't then you are doing it wrong because you should use input in your tests that represents input which you will actually receive in the live application.

• Do the parameter checking separately to make it clear and to reduce indentation:

if(promptNameOrg == null) {
    return "";
}

• I don't see any benefit to using a List<String> over an array. I would just stick to the array so you don't have to wrap it into another collection:

String[] splitPrompts = promptNameOrg.split(";");
String promptName = splitPrompts[splitPrompts.length - 1];

---

I don't see any tests that validate

• Empty input
• Input without ; and \\

Answer 3

Looking at the extractSimplePromptName there are some things that can be improved:

• Return early.

OK, I think that's it actually. Let me show you.

private String extractSimplePromptName(String promptNameOrg) {
    String simplePromptName = "";
    if (promptNameOrg == null) {
        return "";
    }
    if (promptNameOrg.contains(";") || promptNameOrg.contains("\\")) {
        List<String> splitPrompts = Arrays.asList(promptNameOrg.split(";"));

        String promptName = splitPrompts.get(splitPrompts.size() - 1);
        if (promptName.contains("\\")) {
            String[] split = promptName.split("\\\\");
            List<String> splitPromptName = Arrays.asList(split);
            simplePromptName = splitPromptName.get(splitPromptName.size() - 1);
        } else {
            simplePromptName = promptName;
        }
    } else {
        simplePromptName = promptNameOrg;
    }
    return simplePromptName;
}

See what I did there? I magically reduces nearly all your indentation by one step (while keeping a correct indentation). This is done by switching the condition for the if statement and returning inside that if.

I'll do that again with if (promptNameOrg.contains(";") || promptNameOrg.contains("\\")) { (just be sure that you reverse it correctly when you do this with an if-statement that contains || or &&) `

private String extractSimplePromptName(String promptNameOrg) {
    String simplePromptName = "";
    if (promptNameOrg == null) {
        return "";
    }
    if (!promptNameOrg.contains(";") && !promptNameOrg.contains("\\")) {
        return promptNameOrg;
    }
    List<String> splitPrompts = Arrays.asList(promptNameOrg.split(";"));

    String promptName = splitPrompts.get(splitPrompts.size() - 1);
    if (promptName.contains("\\")) {
        String[] split = promptName.split("\\\\");
        List<String> splitPromptName = Arrays.asList(split);
        simplePromptName = splitPromptName.get(splitPromptName.size() - 1);
    } else {
        simplePromptName = promptName;
    }
    return simplePromptName;
}

Now we only have two places left where you set the simplePromptName variable.

if (promptName.contains("\\")) {
    String[] split = promptName.split("\\\\");
    List<String> splitPromptName = Arrays.asList(split);
    simplePromptName = splitPromptName.get(splitPromptName.size() - 1);
} else {
    simplePromptName = promptName;
}

Here we don't need to switch the if-statement, we can just return inside the if as it is and drop the else.

private String extractSimplePromptName(String promptNameOrg) {
    String simplePromptName = "";
    if (promptNameOrg == null) {
        return "";
    }
    if (!promptNameOrg.contains(";") && !promptNameOrg.contains("\\")) {
        return promptNameOrg;
    }
    List<String> splitPrompts = Arrays.asList(promptNameOrg.split(";"));

    String promptName = splitPrompts.get(splitPrompts.size() - 1);
    if (promptName.contains("\\")) {
        String[] split = promptName.split("\\\\");
        List<String> splitPromptName = Arrays.asList(split);
        return splitPromptName.get(splitPromptName.size() - 1);
    }
    return promptName;
}

This method does exactly the same as your original one, but indentation has been drastically reduced (which often helps readability) and some special-cases are dealt with first.

Answer 4

1. Your test procedures aren't named very well. The name of the procedure should tell us what we're testing for. Labeling them as 1,2,3 means we have to look at the test code to figure out which test failed. Make the names clear and meaningful, even if they're abnormally long. That's okay for tests.

2. It doesn't look like you've covered all of the possible test cases. It looks like it's possible for your input to have two consecutive slashes, but you don't test that possibility. You only test if there's one.

   String[] split = promptName.split("\\\\");
   

Answer 5

You could use a parameterized test:

import static org.junit.Assert.assertEquals;

import java.util.Arrays;
import java.util.Collection;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

@RunWith(Parameterized.class)
public class MyParameterizedClassTest {

  private String promptNameOrg;

  public MyParameterizedClassTest(String testParameter, String expectedParameter) {
    this.promptNameOrg = testParameter;
    this.expected = expectedParameter;
  }

  // creates the test data
  @Parameters
  public static Collection<Object[]> data() {
    Object[][] data = new Object[][] { { "20129142\\1234", "1234" }, { "80808080\\159;20129142\\1234", "1234" } };
    return Arrays.asList(data);
  }

  @Test
  public void extractSimplePromptNameFindsLastPartOfString() {
    String simplePromptName = extractSimplePromptName(this.promptNameOrg);
    Assert.assertEquals(this.expected, simplePromptName);
  }

  // extractSimplePromptName omitted
}

There can only be one @Test in such a class, though.

More info can be found here.