2
\$\begingroup\$

Background

Given a string (s) and an offset into that string (offset), find the whole word that can be found at the given offset. (Helpful for auto-completing text.)

Code

Consider:

import static java.lang.Character.isSpaceChar;

public final class TestWordParsing {

  public TestWordParsing() {
  }

  public void run() {
    final String p = "Hello World!";

    test( getWordAt( p, 0 ).equals( "Hello" ), 0 );
    test( getWordAt( p, 3 ).equals( "Hello" ), 3 );
    test( getWordAt( p, 5 ).equals( "Hello" ), 5 );
    test( getWordAt( p, 6 ).equals( "World!" ), 6 );
    test( getWordAt( p, p.length() ).equals( "World!" ), p.length() );

    // Should fail.
    test( getWordAt( p, 0 ).equals( "World!" ), 0 );
  }

  /**
   * Given an arbitrary offset into a string, this returns the word at that
   * index. The inputs and outputs include:
   *
   * <ul>
   * <li>surrounded by space: <code>hello | world!</code> ("");</li>
   * <li>end of word: <code>hello| world!</code> ("hello");</li>
   * <li>start of a word: <code>hello |world!</code> ("world!");</li>
   * <li>within a word: <code>hello wo|rld!</code> ("world!");</li>
   * <li>end of a paragraph: <code>hello world!|</code> ("world!");</li>
   * <li>start of a paragraph: <code>|hello world!</code> ("hello!"); or</li>
   * <li>after punctuation: <code>hello world!|</code> ("world!").</li>
   * </ul>
   *
   * @param s The string to scan for a word.
   * @param offset The offset within s to begin searching for the nearest word
   * boundary, must not be out of bounds of s.
   *
   * @return The word in s at the offset.
   *
   * @see getWordBegan( String, int )
   * @see getWordEnded( String, int )
   */
  public String getWordAt( final String s, final int offset ) {
    final int posBegan = getWordBegan( s, offset );
    final int posEnded = getWordEnded( s, offset );

    return s.substring( posBegan, posEnded );
  }

  /**
   * Returns the index into s where a word begins.
   *
   * @param s A non-null String.
   * @param offset Index into s to begin searching backwards for a word.
   *
   * @return The index where a word begins.
   */
  public int getWordBegan( final String s, int offset ) {
    while( offset > 0 && isWordCharacter( s.charAt( offset - 1 ) ) ) {
      offset--;
    }

    return offset;
  }

  /**
   * Returns the index into s where a word ends.
   *
   * @param s A non-null String.
   * @param offset Index into s to begin searching forwards for a word.
   *
   * @return The index where a word ends.
   */
  public int getWordEnded( final String s, int offset ) {
    final int length = s.length();

    while( offset < length && isWordCharacter( s.charAt( offset ) ) ) {
      offset++;
    }

    return offset;
  }

  /**
   * Returns true if the given character can be reasonably expected to be part
   * of a word, including punctuation marks.
   *
   * @param c The character to compare.
   *
   * @return false The character is a space character.
   */
  private boolean isWordCharacter( char c ) {
    return !isSpaceChar( c );
  }

  /**
   * Poor man's unit testing.
   *
   * @param result Whether the test passed.
   * @param index Index into the paragraph that was tested.
   */
  private void test( boolean result, final int index ) {
    System.out.printf( "%s: %d\n", result ? "Passed" : "Failed", index );
  }

  public static void main( final String args[] ) {
    (new TestWordParsing()).run();
  }
}

Expected Output

The code produces (as expected):

Passed: 0
Passed: 3
Passed: 5
Passed: 6
Passed: 12
Failed: 0

Additional Information

  • The phrase Hello--K&R--World! is three distinct words, which should return Hello, K&R, or World!, depending on the initial offset. However, that condition wasn't stated originally.
  • The index for the start of the word within the string is required because the word will be replaced with different text.

Question

How can the code be improved?

  • Eliminate duplication between getWordBegan and getWordEnded?
  • Considerations for internationalization?
  • Optimizations?
  • Safeguards around invalid indexes?
  • Readability of comments (e.g., preconditions, postconditions)?
\$\endgroup\$
2
\$\begingroup\$

pros

  • you have automated tests with your code.
  • you use proper naming
  • you respect single abstraction layer principle
  • you provided a SSCCE

Cons

  • you use rather hard to read constructions (isWordCharacter( s.charAt( offset - 1 ) ) is not too bad but could be improved...)
  • chose better names for your identifiers, there is no need for single character names.

Eliminate duplication between getWordBegan and getWordEnded?

One solution could be to use regular expressions in a creative way:

public int getWordStarted(final String s, int offset) {
    Matcher matcher = findBorder("\\w+$", s.substring(0, offset));
    return matcher.start();
}

public int getWordEnded(final String s, int offset) {
    Matcher matcher = findBorder("^\\w+", s.substring(offset));
    return offset+matcher.end();
}

private Matcher findBorder(String wordBorderPattern, String substring) {
    Pattern startOfWord = Pattern.compile(wordBorderPattern);
    Matcher matcher = startOfWord.matcher(substring);
    // when position is not inside a word 'matcher.find()' returns false!
    matcher.find();
    return matcher;
}

here is an alternative solution without the need of finding start/end index separately. This implementation fails your test test(getWordAt(p, p.length()).equals("World!"), p.length()); but IMHO this is correct since index p.length() is indeed outside the string...:

public String getWordAt(final String s, final int offset) {
    checkRange(s, offset);
    Matcher wordMatcher = findWordAtIndex(s, offset);
    checkOffsetPointedToWord(offset, wordMatcher);
    return wordMatcher.group();
}

private void checkOffsetPointedToWord(final int offset, Matcher wordMatcher) {
    if (wordMatcher.start() > offset) {
        throw new IllegalArgumentException(offset + " does not point to a word!");
    }
}

private Matcher findWordAtIndex(final String s, final int offset) {
    Pattern wordPatern = Pattern.compile("\\w+", Pattern.CASE_INSENSITIVE);
    Matcher wordMatcher = wordPatern.matcher(s);
    while (wordMatcher.find() && wordMatcher.end() < offset) {
        ; // just loop
            // we could also write the semicolon at the end of the while
            // without the '{}' block but I'd rather show this NOOP
            // explicitly.
    }
    return wordMatcher;
}

private void checkRange(final String s, final int offset) {
    // we need to check the range explicitly because the loop
    // will not throw an exception.
    if (0 > offset || s.length() <= offset) {
        throw new IllegalArgumentException(offset + " is outside String");
    }
}

Considerations for internationalization?

the char datatype is UTF-16.
Any charset conversions must be done when converting the input into a Java String object.

Optimizations?

Never optimize for performance unless you have a prove, that a particular piece of code is a performance problem!

Safeguards around invalid indexes?

Invalid indexes throw exceptions. Catch them and convert them into user friendly error messages. Range checks clutter the code and hide the important implementation details.

Someone may argue that this is flow control which should not be done with exceptions. But IMHO this this is a corner case where I tend to use Exceptions rather than a bunch of if/else cascades at the top of the methods...

Readability of comments (e.g., preconditions, postconditions)?

for my taste this are to much comments for this little code.

I'd be OK with this comments if they where at interface methods. There we need good comments to help the future implementer to understand the contract behind the method.

But on implementations themselves I'd rather see no comments at all. There comments should explain why the code is the way it is, not what it does. So in your comments I'd only keep the assumption, that the parameters must not be null.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ @DaveJarvis sorry, my magic glass bowl kept that from me... ;o) \$\endgroup\$ – Timothy Truckle Nov 20 '16 at 1:49
  • \$\begingroup\$ "to produce a minimal working example for posting." the method isSpaceChar() is missing, so it is not a SSCCE (or MWE neither)... \$\endgroup\$ – Timothy Truckle Nov 20 '16 at 12:05
  • \$\begingroup\$ FWIW, the idea to use regex to grab word start and end is nice, but it doesn't scale well into international languages. Even German or French (or Spanish, or basically everything not English) don't work, because their word-characters contain more than just [a-zA-Z_0-9] \$\endgroup\$ – Vogel612 Nov 20 '16 at 13:20
  • \$\begingroup\$ "but it doesn't scale well into international languages" as long as the implementation of \w is adjusted to C-Names we could change the regex to be more flexible: "[^\\s]+" (docs.oracle.com/javase/7/docs/api/java/util/regex/Pattern.html) \$\endgroup\$ – Timothy Truckle Nov 20 '16 at 13:43

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.