5
\$\begingroup\$

My goal is to return a specific sentence in a story that contains a given word. Returns null if the word is not in the story. I'm ok with returning the first occurrence if there are multiples. Is there a more efficient or cleaner way to do this?

public static String getSentence(String text, String word) {
    String sentence = "";
    if (text.toLowerCase().contains(word)) {
        if (text.contains(".")) {  //Are there sentences terminating in a period?
            int loc = text.toLowerCase().indexOf(word);
            int a = loc;
            while (a >= 0) {
                if (text.charAt(a) == '.' || a == 0) {
                    sentence = text.substring(a,loc);
                    a = 0;
                }
                a--;
            }
            a = loc + word.length();
            while (a <= text.length()) {
                if (text.charAt(a) == '.' || a == text.length()) {
                    sentence += text.substring(loc,a+1);
                    a = text.length()+1;
                }
                a++;
            }
            return sentence;
        } else {
            return text;      //If no period, return full text
        }
    } else {
        return null;
    }
}

FYI - I'm implementing this in Android, so I don't believe I have access to Java 8.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ getSentence("abc. hello", "hello") throws a StringIndexOutOfBoundsException. Is the code meant to deal with cases like this? \$\endgroup\$
    – mjolka
    May 12, 2015 at 2:28
  • \$\begingroup\$ Good catch. It should. \$\endgroup\$
    – Scott
    May 12, 2015 at 10:57

2 Answers 2

8
\$\begingroup\$

There are a few things of interest with your solution. Firstly, it is a very literal implementation of the problem to solve, and I worry that it is too literal. For example, are you sure that sentences end with just a period .? Is it not a period and some whitespace? Is a URL like example.com two sentences?

The second issue I have is the blind trust you have in the inputs. You happily convert the input text to lower-case (too often, actually), but you do not convert the word to lower-case. If someone gives an upper-case word, you'll never find it.

I would prefer a more zen approach, using regular expressions... actually, just a split, and some Java 8 niceness.

private static final Pattern END_OF_SENTENCE = Pattern.compile("\\.\\s+");

public static String getSentence(String text, String word) {
    final String lcword = word.toLowerCase();
    return END_OF_SENTENCE.splitAsStream(text)
            .filter(s -> s.toLowerCase().contains(lcword))
            .findAny()
            .orElse(null);
}

Why is that better? Well, it streams the text in the form of sentences, and then finds the first match in a sentence. If there are no sentences, it matches the whole thing.

Note that the same principles can be used with a non-streaming approach. Split by sentences, then find the first match.

In an android environment, you could do:

private static final Pattern END_OF_SENTENCE = Pattern.compile("\\.\\s+");

public static String getSentence(String text, String word) {
    final String lcword = word.toLowerCase();
    for (String sentence : END_OF_SENTENCE.split(text)) {
        if (sentence.toLowerCase().contains(lcword)) {
            return sentence;
        }
    }
    return null;
}

Note that the results from the above code may, or may not include the terminating period. If the match is in the last sentence of a text, and that text ends with a period, then the period may be returned as part of the result. If there is a match in the middle of the text, then the period will not be included.

\$\endgroup\$
6
  • \$\begingroup\$ Wouldn't a regex like "(?<=\\.)\\s+" make it more sane w.r.t. to the period? It'd keep the period with its sentence and split on the following blanks. \$\endgroup\$
    – maaartinus
    May 12, 2015 at 4:43
  • \$\begingroup\$ This code looks so pretty - however I've discovered I'm not able to take advantage of Java 8 - I'm implementing this in Android. So I can't use splitAsStream unfortunately. I'll update the question to specify. \$\endgroup\$
    – Scott
    May 12, 2015 at 14:38
  • \$\begingroup\$ @Scott - updated with a non-streams version \$\endgroup\$
    – rolfl
    May 12, 2015 at 14:42
  • \$\begingroup\$ Works great! Thank you! I see that the s+ checks for a space afterwards - Is it too much to ask how do I add a check for a quote afterwards? Also ! and ? as opposed to a period? \$\endgroup\$
    – Scott
    May 12, 2015 at 15:14
  • 1
    \$\begingroup\$ @Scott - Got a second to chat about that option in the 2nd monitor chat room - it needs a tweak? \$\endgroup\$
    – rolfl
    May 12, 2015 at 23:27
2
\$\begingroup\$

Toooooo complicated. And a bit inefficient, e.g., text.toLowerCase() gets computed three times. And you're doing

if (text.toLowerCase().contains(word)) {
    if (text.contains(".")) {  //Are there sentences terminating in a period?
        int loc = text.toLowerCase().indexOf(word);

which could be simply

    if (text.contains(".")) {  //Are there sentences terminating in a period?
        int loc = text.toLowerCase().indexOf(word);
        if (loc == -1) {
            return null;
        }

Similarly, you should get rid of text.contains("."). If there's none, a subsequent search will tell you.


I'd go for something like this:

  • find (the first occurrence of) the word
  • if not found, return null
  • find the preceding .
  • the sentence start is at the index following it (or 0 if not found)
  • find the following .
  • the sentence end is at the index of the dot (or text.length() if not found)
  • return the sentence as substring

But this is still too complicated. Something like

Matcher m = Pattern.compile(
        "[^.]*" + Pattern.quote(word) + "[^.]*\\.?",
        Pattern.CASE_INSENSITIVE)
    .matcher(text);
return m.find() ? m.group().trim() : null;

does about the same as your code (untested). It looks for the first sequence of non-periods containing word (and extending as much as possible). The terminating period gets included, if any. As a sentence neither starts nor ends with blanks, the result gets trimmed.

The word gets quoted just in case it's not a word, but something like $20.


For the definition of sentence as something ending with a period followed by blanks, the regex would get rather convoluted, but it's possible as well. The pattern changes to something like

"([^.]|\\.+[^ .])*" + Pattern.quote(word) + "([^.]|\\.+[^ .])*\\.?"

employing an old trick: Accept a non-period ([^.]) or a period (or more of them) followed by a non-space-non-period (\\.+[^. ]). Any number of times (*).

\$\endgroup\$
2
  • \$\begingroup\$ This is returning matchFound=false. I'm so far not able to implement this solution. \$\endgroup\$
    – Scott
    May 12, 2015 at 14:42
  • \$\begingroup\$ @Scott Thanks, fixed by replacing match by find. \$\endgroup\$
    – maaartinus
    May 12, 2015 at 15:22

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.