22
\$\begingroup\$

Description:

Bob is a lackadaisical teenager. In conversation, his responses are very limited.

Bob answers 'Sure.' if you ask him a question.

He answers 'Whoa, chill out!' if you yell at him.

He says 'Fine. Be that way!' if you address him without actually saying anything.

He answers 'Whatever.' to anything else.

Code:

import java.util.Optional;

enum SentenceType {
  Question, Yell, Silence, Misc
}

public class Bob {
  public String hey(String sentence) {
    String trimmedSentence = Optional.ofNullable(sentence).orElse("").trim();
    SentenceType classifiedSentence = classify(trimmedSentence);

    if (classifiedSentence == SentenceType.Silence) {
      return "Fine. Be that way!";
    } else if (classifiedSentence == SentenceType.Question) {
      return "Sure.";
    } else if (classifiedSentence == SentenceType.Yell) {
      return "Whoa, chill out!";
    }
    return "Whatever.";

  }

  private SentenceType classify(String sentence) {
    if (sentence.isEmpty()) {
      return SentenceType.Silence;
    } else if (isAllLettersUpperCase(sentence)) {
      return SentenceType.Yell;
    } else if (isQuestion(sentence)) {
      return SentenceType.Question;
    }
    return SentenceType.Misc;
  }

  private boolean isAllLettersUpperCase(String sentence) {
    char[] charArray = sentence.toCharArray();
    boolean hasLetters = false;
    for (char ch : charArray) {
      if (Character.isLetter(ch)) {
        hasLetters = true;
        if (Character.isLowerCase(ch)) {
          return false;
        }
      }
    }
    return hasLetters && true;
  }

  private boolean isQuestion(String sentence) {
    return (sentence.charAt(sentence.length() - 1) == '?') ? true : false;
  }
}

Test Suite:

import org.junit.Test;

import static org.junit.Assert.*;

public class BobTest {
    private final Bob bob = new Bob();

    @Test
    public void saySomething() {
        assertEquals(
            "Whatever.",
            bob.hey("Tom-ay-to, tom-aaaah-to.")
        );
    }

    @Test
    public void shouting() {
        assertEquals(
            "Whoa, chill out!",
            bob.hey("WATCH OUT!")
        );
    }

    @Test
    public void askingAQuestion() {
        assertEquals(
            "Sure.",
            bob.hey("Does this cryogenic chamber make me look fat?")
        );
    }

    @Test
    public void askingANumericQuestion() {
        assertEquals(
            "Sure.",
            bob.hey("You are, what, like 15?")
        );
    }

    @Test
    public void talkingForcefully() {
        assertEquals(
            "Whatever.",
            bob.hey("Let's go make out behind the gym!")
        );
    }

    @Test
    public void usingAcronymsInRegularSpeech() {
        assertEquals(
            "Whatever.", bob.hey("It's OK if you don't want to go to the DMV.")
        );
    }

    @Test
    public void forcefulQuestions() {
        assertEquals(
            "Whoa, chill out!", bob.hey("WHAT THE HELL WERE YOU THINKING?")
        );
    }

    @Test
    public void shoutingNumbers() {
        assertEquals(
            "Whoa, chill out!", bob.hey("1, 2, 3 GO!")
        );
    }

    @Test
    public void onlyNumbers() {
        assertEquals(
            "Whatever.", bob.hey("1, 2, 3")
        );
    }

    @Test
    public void questionWithOnlyNumbers() {
        assertEquals(
            "Sure.", bob.hey("4?")
        );
    }

    @Test
    public void shoutingWithSpecialCharacters() {
        assertEquals(
            "Whoa, chill out!", bob.hey("ZOMG THE %^*@#$(*^ ZOMBIES ARE COMING!!11!!1!")
        );
    }

    @Test
    public void shoutingWithUmlauts() {
        assertEquals(
            "Whoa, chill out!", bob.hey("\u00dcML\u00c4\u00dcTS!")
        );
    }

    @Test
    public void calmlySpeakingWithUmlauts() {
        assertEquals(
            "Whatever.", bob.hey("\u00dcML\u00e4\u00dcTS!")
        );
    }

    @Test
    public void shoutingWithNoExclamationMark() {
        assertEquals(
            "Whoa, chill out!", bob.hey("I HATE YOU")
        );
    }

    @Test
    public void statementContainingQuestionMark() {
        assertEquals(
            "Whatever.", bob.hey("Ending with ? means a question.")
        );
    }

    @Test
    public void prattlingOn() {
        assertEquals(
            "Sure.", bob.hey("Wait! Hang on. Are you going to be OK?")
        );
    }

    @Test
    public void silence() {
        assertEquals(
            "Fine. Be that way!", bob.hey("")
        );
    }

    @Test
    public void prolongedSilence() {
        assertEquals(
            "Fine. Be that way!", bob.hey("    ")
        );
    }
}

Question

  1. At first I was skeptical about the unicode issue but I think library handles the uppercase and lowercase unicode well.
  2. It took me much time to write so simple looking isAllLettersUpperCase I am still doubtful that I have written it well and it handles all corner cases, how to write such method with more confidence?
  3. There is code smell in the order of method called i.e. between isAllLettersUpperCase and isQuestion by changing the order I would break tests. Hopefully there is a scope of handling this better.
  4. The overall idea was to reduce coupling as much as possible, hopefully I handled it fine to some extent by using enums.

PS: For more details refer here.

\$\endgroup\$
  • 2
    \$\begingroup\$ not sure if I miss something, so I make it a comment. How about sentence.equals(sentence.toUpperCase())? \$\endgroup\$ – Bene May 2 '16 at 18:01
  • \$\begingroup\$ @Malachi docs.oracle.com/javase/7/docs/api/java/lang/… \$\endgroup\$ – Bene May 2 '16 at 19:38
  • \$\begingroup\$ duh....ROFL I hate Mondays, @Bene. Character.isUpperCase() takes a single character. \$\endgroup\$ – Malachi May 2 '16 at 19:43
  • \$\begingroup\$ Yes, I was just asking if String.toUpperCase() would work for his isAllLetterUpperCase() method. Or would it return different results than the current method? \$\endgroup\$ – Bene May 2 '16 at 19:45
  • 5
    \$\begingroup\$ O_O With my name and avatar... this can't be a coincidence, can it? \$\endgroup\$ – Bob May 3 '16 at 2:49
24
\$\begingroup\$

Avoid if chains with enumerations

You are using an enumeration for all the possible responses of Bob. This is a great idea, and you can take it a step further. Currently, you have:

SentenceType classifiedSentence = classify(trimmedSentence);

if (classifiedSentence == SentenceType.Silence) {
    return "Fine. Be that way!";
} else if (classifiedSentence == SentenceType.Question) {
    return "Sure.";
} else if (classifiedSentence == SentenceType.Yell) {
    return "Whoa, chill out!";
}
return "Whatever.";

which consists of multiple if statement in order to return the correct output.

Consider making the sentence a property of the enum:

enum SentenceType {
    Question("Sure."),
    Yell("Whoa, chill out!"),
    Silence("Fine. Be that way!"),
    Misc("Whatever.");

    private final String sentence;

    SentenceType(String sentence) {
        this.sentence = sentence;
    }

    public String getSentence() {
        return sentence;
    }
}

This way, you can refactor this block:

SentenceType classifiedSentence = classify(trimmedSentence);
return classifiedSentence.getSentence();

without any ifs.


Regarding your concern of the order of the methods, I don't see it as an issue: it depends on the order only because you decided you write your conditions as early-return. This means that the check to decide if Bob is yelling is pre-supposing that Bob actually said something, which also means that you don't explicitely check that Bob said something to determine if he's yelling. This indeed couples the order but it does simplify the checks involved: otherwise, you would need to add a !sentence.isEmpty() && ... to every condition.

Optional

You are using an Optional for the following:

String trimmedSentence = Optional.ofNullable(sentence).orElse("").trim();

This is over-kill, it would be simpler (and perhaps clearer) to just use a ternary operator:

String trimmedSentence = sentence == null ? "" : sentence.trim();

Stream API

You could refactor your isAllLettersUpperCase method to the following

private boolean isAllLettersUpperCase(String sentence) {
    return sentence.chars().anyMatch(Character::isLetter) && 
            sentence.chars().filter(Character::isLetter).allMatch(Character::isUpperCase);
}

which may be easier to read: it returns true if there at least one letter and all letters are uppercase.

\$\endgroup\$
  • 1
    \$\begingroup\$ I really like the idea of augmenting enum. \$\endgroup\$ – CodeYogi May 2 '16 at 18:06
  • \$\begingroup\$ can't you just do the second half of that conditional for the isAllLettersUpperCase return? \$\endgroup\$ – Malachi May 2 '16 at 18:23
  • 1
    \$\begingroup\$ @Malachi No, allMatch will return true if there are no elements, so without the first half, it would return true if there are no letters (refer here). \$\endgroup\$ – Tunaki May 2 '16 at 18:56
  • \$\begingroup\$ Awesome, thank you. I am not familiar with very many Java tools. \$\endgroup\$ – Malachi May 2 '16 at 18:58
  • \$\begingroup\$ @CodeYogi I didn't add it (because it would maybe be too much) but you could also refactor the classify method by adding each check inside a Predicate<String> for each enum. \$\endgroup\$ – Tunaki May 2 '16 at 19:30
8
\$\begingroup\$
  private boolean isQuestion(String sentence) {
    return (sentence.charAt(sentence.length() - 1) == '?') ? true : false;
  }

Here you could actually write this a little simpler, without the ternary.

return sentence.charAt(sentence.length() - 1) == '?';

You are returning true or false to return true or false. Just do it, once.


You can reduce the amount of indentation on this

  private boolean isAllLettersUpperCase(String sentence) {
    char[] charArray = sentence.toCharArray();
    boolean hasLetters = false;
    for (char ch : charArray) {
      if (Character.isLetter(ch)) {
        hasLetters = true;
        if (Character.isLowerCase(ch)) {
          return false;
        }
      }
    }
    return hasLetters && true;
  }

I would change the name of the method to areAllLettersUpperCase (just sounds better). in this method, it doesn't make much sense to

return hasLetters && true;

because it really comes out to

return hasLetters;

I would separate the has letters functionality into its own method, so that if there are no letters I could do something else entirely.

private boolean hasLetters(String sentence) {
    char[] charArray = sentence.toCharArray();
    for (char ch : charArray) {
        if (Character.isLetter(ch)) return true;
    }
    return false;
}

then you can call it like this in the areAllLettersUpperCase method.

private boolean isAllLettersUpperCase(String sentence) {
    char[] charArray = sentence.toCharArray();
    if (!hasLetters(sentence)) return false;
    for (char ch : charArray) {
        if (Character.isLowerCase(ch)) {
            return false;
        }
    }
    return true;
}

But, I would use the fact that the sentence doesn't have any letters to make the bot say something else entirely, move it out of this method and rename it accordingly.

By separating the check for letters in the sentence, you will be following the Single Responsibility Principle (SRP). Each method should do exactly one thing. The calling class should call the check for the absence of letters before it calls a check for a sentence in all CAPS.

SRP is one of the pieces of writing SOLID Code as well.

\$\endgroup\$
  • \$\begingroup\$ facepalm! I always forget that. \$\endgroup\$ – CodeYogi May 2 '16 at 17:33
  • \$\begingroup\$ @CodeYogi, lol I do it all the time! \$\endgroup\$ – Malachi May 2 '16 at 17:55
  • \$\begingroup\$ I thought to make a separate method like HasLetters but that needed to iterate though the whole string again, isn't it a performance hit? \$\endgroup\$ – CodeYogi May 2 '16 at 18:04
  • \$\begingroup\$ if you read in an entire book, yes it would be, but for a normal back and forth conversation you won't notice, unless you are running a couple thousand instances at the same time to test for a performance issue. \$\endgroup\$ – Malachi May 2 '16 at 18:21
  • 1
    \$\begingroup\$ @Malachi Yes, that's what I figured :) \$\endgroup\$ – Tunaki May 2 '16 at 19:08
3
\$\begingroup\$

Just to augment the other answers:

  public String hey(String sentence) {
    String trimmedSentence = Optional.ofNullable(sentence).orElse("").trim();
    SentenceType classifiedSentence = classify(trimmedSentence);

    if (classifiedSentence == SentenceType.Silence) {
      return "Fine. Be that way!";
    } else if (classifiedSentence == SentenceType.Question) {
      return "Sure.";
    } else if (classifiedSentence == SentenceType.Yell) {
      return "Whoa, chill out!";
    }
    return "Whatever.";
  }

You can write this if-elseif-else sequence as a switch block:

    switch (classifiedSentence) {
    case Silence:
      return "Fine. Be that way!";
    case Question:
      return "Sure.";
    case Yell:
      return "Whoa, chill out!";
    default:
      return "Whatever.";
    }

This saves you (and your fellow readers) a few words.

Instead of this:

  private boolean isQuestion(String sentence) {
    return (sentence.charAt(sentence.length() - 1) == '?') ? true : false;
  }

Just write this:

  private boolean isQuestion(String sentence) {
    return sentence.endsWith("?");
  }
\$\endgroup\$
  • \$\begingroup\$ I tried switch but I got error saying the expression must resolve to int. \$\endgroup\$ – CodeYogi May 3 '16 at 3:30
  • \$\begingroup\$ @CodeYogi: Since Java 1.5, the switch statement accepts enums, and since Java 1.7, it also accepts strings. Please don’t tell me that you are developing in Java 1.4 mode. \$\endgroup\$ – Roland Illig May 3 '16 at 20:14
  • \$\begingroup\$ No, its java 7., maybe I was doing something wrong. \$\endgroup\$ – CodeYogi May 4 '16 at 2:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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