Home Original page

java.util.Optional matchers by seregamorph · Pull Request #421 · hamcrest/JavaHamcrest

Thanks for this, @seregamorph! I appreciate the time you've put into this PR. The code is clean and readable, and I think it will be a valuable addition to Hamcrest. If you don't mind, can I request some changes?

Firstly, I'm not keen on the use of anonymous inner classes for published matchers in Hamcrest. It would be great if you re-write them as top level public classes that subclass the same TypeSafeDiagnosingMatcher<Optional<T>> parent

Secondly, I'm not definite about this, but I think it would be nicer to put them in a separate sub-package (e.g. optional)

Thirdly (and finally), I think static factory method names could give a bit more context, to make them more readable when they've been staticly imported. e.g. change isEmpty() to emptyOptional(), and change isPresent() to optionalWithValue().

Putting those points together, we'd end up with something like this:

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.optional.OptionalMatchers.emptyOptional;
import static org.hamcrest.optional.OptionalMatchers.optionalWithValue;
import static org.hamcrest.text.MatchesPattern.matchesPattern;
import org.junit.Test;
import java.util.Optional;

public class OptionalMatchersTest {
    @Test
    public void testEmptyOptional() {
        Optional<Object> actual = Optional.empty();
        // assertThat(actual, isEmpty());
        assertThat(actual, is(emptyOptional()));
        assertThat(actual, not(optionalWithValue()));
    }

    @Test
    public void testOptionalWithValue() {
        Optional<String> actual = Optional.of("Hello, world");
        // assertThat(actual, isPresent());
        assertThat(actual, not(emptyOptional()));
        assertThat(actual, is(optionalWithValue()));
        assertThat(actual, optionalWithValue("Hello, world"));
        assertThat(actual, optionalWithValue(matchesPattern("Hell")));
    }
}

What do you think?