3
\$\begingroup\$

This is a of some recent "search word in multiple files" questions, such as this deleted question and this.

The idea here is to do a case-insensitive word (complete, not partial) search that can also report the line and 'column' number of the search result. For example, if the word is found as the first word on the first row, its location can be represented as row = 0, column = 0, or r0c0.

I have a Position model class to represent a search result, and an interface WordCount that represents how searches can be done and the results reported. A FileWordCount handles the searching of one Path, and a FileWordSearcher (while implementing WordCount too) performs the same search operations on a Set<Path>. For completeness, a TestNG test is also provided.

Questions:

  1. Is there a better way to write the test, other than listing the expected objects literally?
  2. Any other refactoring suggested on the main classes/interfaces?
    • I was pondering if a Set<Position> (backed by a HashSet) is adequate, or should there be an ordering as well using LinkedHashSet. A List provides a predictable iteration/sorted order, but loses the uniqueness semantics.

Position

public final class Position implements Comparable<Position> {
    public static final Comparator<Position> COMPARATOR =
            Comparator.comparing(Position::getRow)
                        .thenComparing(Position::getColumn);

    private final int row;
    private final int column;
    private final int hashCode;

    public Position(int row, int column) {
        this.row = row;
        this.column = column;
        this.hashCode = Objects.hash(row, column);
    }

    public int getRow() {
        return row;
    }

    public int getColumn() {
        return column;
    }

    @Override
    public int hashCode() {
        return hashCode;
    }

    @Override
    public boolean equals(Object o) {
        return o == this || (o instanceof Position && equals((Position) o));
    }

    private boolean equals(Position other) {
        return row == other.row && column == other.column;
    }

    @Override
    public String toString() {
        return "r" + row + "c" + column;
    }

    @Override
    public int compareTo(Position o) {
        return COMPARATOR.compare(this, o);
    }
}

WordCount

public interface WordCount {
    String getName();

    int getCount(String word);

    Set<Position> getPositions(String word);

    default Map<String, Integer> getCountWithName(String word) {
        return Stream.of(word)
                    .collect(Collectors.toMap(k -> getName(), this::getCount));
    }

    default Map<String, Set<Position>> getPositionsWithName(String word) {
        return Stream.of(word)
                    .collect(Collectors.toMap(k -> getName(), this::getPositions));
    }
}

FileWordCount

public final class FileWordCount implements WordCount {
    private static final Pattern SPLITTER = Pattern.compile("\\W+");
    private final String name;
    private final Map<String, Set<Position>> lookup;

    public FileWordCount(Path path) {
        this.name = path.toString();
        this.lookup = Collections.unmodifiableMap(loadFile(path));
    }

    private Map<String, Set<Position>> loadFile(Path path) {
        try (Stream<String> lines = Files.lines(path)) {
            int[] counter = new int[1];
            return aggregate(
                    lines.map(l -> getPositions(counter[0]++, l)),
                    Collector.of(HashSet::new, Set::addAll,
                            (a, b) -> { a.addAll(b); return a; }));
        } catch (IOException e) {
            throw new UncheckedIOException(e);
        }
    }

    private Map<String, Set<Position>> getPositions(int row, String line) {
        int[] counter = new int[1];
        return aggregate(
                SPLITTER.splitAsStream(line)
                        .map(String::toLowerCase)
                        .map(word -> Collections.singletonMap(word,
                                new Position(row, counter[0]++))),
                Collectors.toSet());
    }

    private static <K, U, V> Map<K, Set<V>> aggregate(Stream<Map<K, U>> stream,
                                        Collector<U, ?, Set<V>> downstream) {
        return stream.map(Map::entrySet)
                    .flatMap(Set::stream)
                    .collect(Collectors.groupingBy(Entry::getKey,
                            Collectors.mapping(Entry::getValue, downstream)));
    }

    @Override
    public String getName() {
        return name;
    }

    @Override
    public int getCount(String word) {
        return getPositions(word).size();
    }

    @Override
    public Set<Position> getPositions(String word) {
        return lookup.getOrDefault(word, Collections.emptySet());
    }
}

FileWordSearcher

public final class FileWordSearcher implements WordCount {
    private final Set<Path> inputs;
    private final Set<WordCount> results;

    public FileWordSearcher(Path...paths) {
        this(Arrays.stream(Objects.requireNonNull(paths)));
    }

    public FileWordSearcher(Stream<Path> paths) {
        Objects.requireNonNull(paths);
        this.inputs = paths.collect(toUnmodifiableSet());
        this.results = inputs.stream()
                                .map(FileWordCount::new)
                                .collect(toUnmodifiableSet());
    }

    private static <T> Collector<T, ?, Set<T>> toUnmodifiableSet() {
        return Collectors.collectingAndThen(Collectors.toSet(),
                Collections::unmodifiableSet);
    }

    @Override
    public String getName() {
        return toString();
    }

    @Override
    public int getCount(String word) {
        return results.stream().mapToInt(v -> v.getCount(word)).sum();
    }

    @Override
    public Set<Position> getPositions(String word) {
        return results.stream()
                        .map(v -> v.getPositions(word))
                        .reduce(new HashSet<>(),
                                (a, b) -> { a.addAll(b); return a; });
    }

    @Override
    public Map<String, Integer> getCountWithName(String word) {
        return results.stream()
                .collect(Collectors.toMap(v -> v.getName(),
                                v -> v.getCount(word)));
    }

    @Override
    public Map<String, Set<Position>> getPositionsWithName(String word) {
        return results.stream()
                    .collect(Collectors.toMap(v -> v.getName(),
                                v -> v.getPositions(word)));
    }
}

FileWordSearcherTest

This test searches for the word "light" in the text The Last Question and Leiningen versus the Ants (input files have been stripped of empty lines).

public class FileWordSearcherTest {

    private static final Logger LOG = 
                    LoggerFactory.getLogger(FileWordSearcherTest.class);

    @Test
    public void testSearch() {
        Path source = Paths.get("path", "to", "text");
        Supplier<Stream<Path>> supplier = () -> Stream.of("lastqn", "leiningen")
                                                        .map(v -> v + ".txt")
                                                        .map(source::resolve);
        FileWordSearcher searcher = new FileWordSearcher(supplier.get());
        search(searcher, "light",
                createMap(supplier.get().map(Path::toString).iterator(),
                        Arrays.asList(4, 1).iterator()),
                createMap(supplier.get().map(Path::toString).iterator(),
                        Arrays.asList(toPositions(2, 25, 146, 29, 182, 6, 183, 3),
                                toPositions(58, 61)).iterator()));
    }

    private static void search(WordCount instance, String word,
            Map<String, Integer> expectedCounts,
            Map<String, Set<Position>> expectedPositions) {
        LOG.info("Searching for: {}", word);
        Map<String, Integer> counts = instance.getCountWithName(word);
        assertThat(counts, equalTo(expectedCounts));
        counts.forEach(showMapEntry("{}: {} occurrences"));
        Map<String, Set<Position>> positions = instance.getPositionsWithName(word);
        assertThat(positions, equalTo(expectedPositions));
        positions.forEach(showOrderedMapEntry("{}:\n\t{}", Position.COMPARATOR));
    }

    private static <K, V> BiConsumer<K, V> showMapEntry(String format) {
        return (k, v) -> LOG.info(format, k, v);
    }

    private static <K, V> BiConsumer<K, Set<V>> showOrderedMapEntry(String format,
                                                        Comparator<V> comparator) {
        return (k, v) -> LOG.info(format, k,
                v.stream().sorted(comparator).collect(Collectors.toList()));
    }

    private static <V> Map<String, V> createMap(Iterator<String> keys, 
            Iterator<V> values) {
        Map<String, V> results = new HashMap<>();
        while (keys.hasNext() && values.hasNext()) {
            results.put(keys.next(), values.next());
        }
        if (keys.hasNext() || values.hasNext()) {
            throw new IllegalArgumentException("Not expecting more entries.");
        }
        return results;
    }

    private static Set<Position> toPositions(int... values) {
        if (values.length % 2 != 0) {
            throw new IllegalArgumentException("Not expecting odd number.");
        }
        Set<Position> results = new HashSet<>();
        for (int i = 0; i < values.length; i++) {
            results.add(new Position(values[i], values[++i]));
        }
        return results;
    }
}

Output

INFO : Searching for: light
INFO : path\to\text\lastqn.txt: 4 occurrences
INFO : path\to\text\leiningen.txt: 1 occurrences
INFO : path\to\text\lastqn.txt:
    [r2c25, r146c29, r182c6, r183c3]
INFO : path\to\text\leiningen.txt:
    [r58c61]
PASSED: testSearch
\$\endgroup\$
2
\$\begingroup\$

Don't use the stream api when simpler things exist

default Map<String, Integer> getCountWithName(String word) {
    return Stream.of(word)
                .collect(Collectors.toMap(k -> getName(), this::getCount));
}

default Map<String, Set<Position>> getPositionsWithName(String word) {
    return Stream.of(word)
                .collect(Collectors.toMap(k -> getName(), this::getPositions));
}

While you can use the Stream API here, it is easier to understand when you use Collections.singeletonMap. This returns a map containing 1 key with value.

default Map<String, Integer> getCountWithName(String word) {
    return unmodifiableMap(getName(), getCount(word));
}

default Map<String, Set<Position>> getPositionsWithName(String word) {
    return unmodifiableMap(getName(), getPositions(word));
}

Unneeded variable

private final Set<Path> inputs;
public FileWordSearcher(Stream<Path> paths) {
    Objects.requireNonNull(paths);
    this.inputs = paths.collect(toUnmodifiableSet());
    this.results = inputs.stream()
                            .map(FileWordCount::new)
                            .collect(toUnmodifiableSet());
}

This variable is only used in the constructor of the function, by removing the variable and the unneeded stream() call you can increase the performance and the memory in 1 shot.

public FileWordSearcher(Stream paths) { Objects.requireNonNull(paths); this.results = paths.map(FileWordCount::new) .collect(toUnmodifiableSet()); }

Requirement of Path objects

By the way you implemented file reading, you require the usage of Path objects to your code. This makes unit testing harder as there is no defined location for files, you can only use Class.getResource to get a proper reference to the file.

By changing your system to take a URL you can take advantage of test files directly next to the normal files, and without risking bugs if your testing platform tests after putting the files in a jar.

Throw AssertionError while testing

For any defined failure path of a unit test, you should use AssertionError or Assert.fail() rather than throwing a IllegalArgumentException.

Adding Javadoc

While most developers put the javadoc at the lowest priority, these can help with explaining this. For someone new to your api, it may not been clear that the key from the returned map from getCountWithName represents a the name of the WordCount.

Better testing (your question 1)

You only have 1 testcase, this isn't enough to test the whole application. A other point of testing is that you should limit the things to test in every test, so you can find out what method is at fault, by only looking at the test results.

The following points should be tested:

  • Empty file to search
  • Searching for a file with exact the same content

Examples of small, concrete, on point tests:

@Test
public void emptyInputSearchTest() {
    FileWordSearcher searcher = new FileWordSearcher(Stream.empty());
    assertTrue(searcher.getCountWithName("test").isEmpty());
    assertTrue(searcher.getPositionsWithName("test").isEmpty());
}

@Test
public void FileToSearchMatchInputWordTest() {
    Path p = ....; //Image this path has a file that contains only "test"
    FileWordSearcher searcher = new FileWordSearcher(Stream.of(p));
    int count = searcher.getCountWithName("test").get(p.getName());
    assertTrue(count == 1);
    Set<Position> pos = searcher.getPositionsWithName("test").get(p.getName());
    assertTrue(pos.size() == 1);
    assertTrue(pos.contains(new Position(0,0)));
}

Notice I used assertTrue in the above examples as I dont know how to work with Hamcrest yet....

Return type of getPositions and getCounts (question 2)

If I am searching text, I would expect it to return a set that has a order from first to last match, as this is usually how I present the results to my user. A NavigableSet is perfect for this as it has a iterator() for moving from the start top the end, and a descendingIterator() for the other way. a TreeSet is a commonly used Set for implementing a NavigableSet.

\$\endgroup\$
  • \$\begingroup\$ The suggestion for TreeSet is useful! :) \$\endgroup\$ – h.j.k. Feb 4 '16 at 0:36

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.