1
\$\begingroup\$

Prompt:

You have been given a string s, which is supposed to be a sentence. However, someone forgot to put spaces between the different words, and for some reason, they capitalized the first letter of every word. Return the sentence after making the following amendments:

Put a single space between the words. Convert the uppercase letters to lowercase.

Any suggestions on my code?

String amendTheSentence(String s) {
    StringBuilder modifiedString = new StringBuilder();
    char[] sToArray = s.toCharArray();
    if (Character.isUpperCase(sToArray[0])) {
        modifiedString.append(Character.toLowerCase(sToArray[0]));
    } else {
        modifiedString.append(sToArray[0]);
    }

    for (int i = 1; i < sToArray.length; i++) {
        char currentChar = sToArray[i];

        if (Character.isUpperCase(sToArray[i])) {
           currentChar = Character.toLowerCase(currentChar);
            modifiedString.append(" ");
        } 
        modifiedString.append(currentChar);
    }
    return modifiedString.toString();
}
\$\endgroup\$
2
  • \$\begingroup\$ Could you add some test cases? \$\endgroup\$
    – vnp
    Jan 26, 2018 at 4:10
  • \$\begingroup\$ Your Code fails to meet the requirement since it adds tha space after the upper case letter... \$\endgroup\$ Jan 26, 2018 at 8:24

2 Answers 2

3
\$\begingroup\$

Thanks for sharing the code.

This is quite good for a first attempt (except the bug).

Things to consider:

Odd ball solution

This means that you do the same thing in different ways.

  1. use of variable vs direct array access

    when you handle the first letter you directly access it using array index access sToArray[0] whereas for all the other letters you first put the actual char into a local variable currentChar but you do not use it in the if statement. You should do the same at all places.

  2. similar logic that could be the same

    this is a bit trickier and may need some experience, but:
    The difference between the handling of the first letter and all the other letters is that you don't put a space before it. Having in mind that there is an empty string ("") in Java putting that instead of the space is just a configuration, a different value to use and not a different behavior needing different instructions (or their order). Including point 1 your code could look like this:
    (I intentionally kept your bug!)

    String amendTheSentence(String s) {
        StringBuilder modifiedString = new StringBuilder();
        char[] sToArray = s.toCharArray();
        String separator = "";
        char currentChar = sToArray[0];
    
        if (Character.isUpperCase(currentChar)) {
            currentChar = Character.toLowerCase(currentChar);
            modifiedString.append(separator );
        } 
        modifiedString.append(currentChar);
    
        for (int i = 1; i < sToArray.length; i++) {
            String separator = " ";
            char currentChar = sToArray[i];
    
            if (Character.isUpperCase(currentChar)) {
                currentChar = Character.toLowerCase(currentChar);
                modifiedString.append(separator);
            } 
            modifiedString.append(currentChar);
        }
        return modifiedString.toString();
    }
    

    Now you have 4 lines which are repeated exactly the same before the loop and inside the loop. This 4 lines can be extracted to a parameterized method:

    private void convertAndAppendTo(StringBuilder modifiedString, char currentChar,
                                    String separator){
        if (Character.isUpperCase(currentChar)) {
            currentChar = Character.toLowerCase(currentChar);
            modifiedString.append(separator ); 
        } 
        modifiedString.append(currentChar);
    }
    

    and your original method changes to this:

    String amendTheSentence(String s) {
        StringBuilder modifiedString = new StringBuilder();
        char[] sToArray = s.toCharArray();
        convertAndAppendTo(modifiedString,  sToArray[0], "");
    
        for (int i = 1; i < sToArray.length; i++) {
            convertAndAppendTo(modifiedString,  sToArray[i], " ");
        }
        return modifiedString.toString();
    }
    

Magic numbers

In your code the 0 is a magic number and should be declared as a constant with a meaningful name like this:

private static final int FIRST_LETTER_INDEX = 0;

String amendTheSentence(String s) {
    StringBuilder modifiedString = new StringBuilder();
    char[] sToArray = s.toCharArray();
    convertAndAppendTo(modifiedString,  sToArray[FIRST_LETTER_INDEX], "");
// ...

I for myself would also tread literal strings as magic numbers and convert them into constants:

private static final int FIRST_LETTER_INDEX = 0;
private static final String FIRST_SEPARATOR = "";
private static final String INTER_WORD_SEPARATOR = " ";

String amendTheSentence(String s) {
    StringBuilder modifiedString = new StringBuilder();
    char[] sToArray = s.toCharArray();
    convertAndAppendTo(modifiedString,  sToArray[FIRST_LETTER_INDEX], FIRST_SEPARATOR);
// ...

Naming

Finding good names is the hardest part in programming. So always take your time to think carefully of your identifier names.

Get your names from the problem domain, not from the technical solution.

You have a variable sToArray that stores the individual chars. The name tells something about the type of the variable but it should tell something about its purpose instead. So a better name might be individualLetters or just letters.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you, I am constantly working on improving how I name variables. \$\endgroup\$
    – TheLearner
    Jan 28, 2018 at 23:16
2
\$\begingroup\$
/**
 * Fixed the sentence by putting a separator before an upper case character. Then, convert all characters to lowercase. 
 * @param unseparatedString - the string to convert
 * @param separator - the words divider
 * @return the sentence that is divided into words.
 */
private String parseStringToSentence(String unseparatedString, char separator) {
    StringBuilder stringBuilder = new StringBuilder();
    char[] characters = unseparatedString.toCharArray();

    for(char character : characters) {
        if(Character.isUpperCase(character)) {
            stringBuilder.append(separator);
        }

        stringBuilder.append(character);
    }

    return stringBuilder.toString().toLowerCase().trim();
}
  1. Always put a documentation comment on methods and classes. Your program might be short now but when it grew, it will lead to confusion.
  2. Make sure your naming of variable is descriptive.
  3. For this program, since I am just accessing the array consecutively, it is better to use a foreach loop. It's short and simple.
  4. Since you will be converting all the characters into lower case, you can just simply use the String.toLowerCase() method on the end rather than putting it on an if statement.
  5. I trim the final sentence to remove the extra space in the front.
\$\endgroup\$
2
  • \$\begingroup\$ comments: As a beginner it might be OK to add the requirements as header comment. Also we should comment interfaces what purpose they serve and what contract the implementer has to comply with. Beside this comments should tell why the code is like it is. Th document *what the code does" you should look for better names. \$\endgroup\$ Jan 28, 2018 at 10:25
  • \$\begingroup\$ foreach loop: I disagree with this suggestion! A forech loop cannot skip any element of the collection. That means in this case that you need a if inside the loop just to decide if this is the loops first iteration. I really doubt that this is more readable then explicitly handle the first index before the loop and then skipping it in the loop. \$\endgroup\$ Jan 28, 2018 at 10:33

Your Answer

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

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