2
\$\begingroup\$

Background

(these are the bits technically not for review, but feel free to point out any minor points)

An Entity class can be identified by a name and at least one alias:

public final class Entity {

    private final String name;
    private final Set<String> aliases;

    public Entity(String name) {
        this(name, name);
    }

    public Entity(String name, String... aliases) {
        if (aliases == null || aliases.length == 0) {
            throw new IllegalArgumentException("At least one alias expected.");
        }
        this.name = name;
        this.aliases = ImmutableSet.copyOf(aliases);
    }

    public String getName() {
        return name;
    }

    public Set<String> getAliases() {
        return aliases;
    }
}

A Container interface describes how to find by a name or an alias:

public interface Container {

    Optional<Entity> findByName(String name);
    Optional<Entity> findByAlias(String name);
}

For the purpose of the comparison below, it is safe to assume that an Entity can be uniquely identified by a name or alias in a Container.


Lookup implementation #1

This performs the mapping and comparison on-the-fly.

Advantages

  • Method implementations can be read fluently
  • Opens up the possibility of additional searching criteria by using a different BiPredicate, e.g. using String::equalsIgnoreCase for a case-insensitive name search

Unsure about

  • Performance, is it optimal?

 

public final class EntityContainer implements Container {

    private final Set<Entity> entities;

    public EntityContainer(Entity... entities) {
        if (entities == null || entities.length == 0) {
            throw new IllegalArgumentException("At least one entity expected.");
        }
        this.entities = ImmutableSet.copyOf(entities);
    }

    @Override
    public Optional<Entity> findByName(String name) {
        return findBy(Entity::getName, Object::equals, name);
    }

    @Override
    public Optional<Entity> findByAlias(String alias) {
        return findBy(Entity::getAliases, Set::contains, alias);
    }

    private <X, Y> Optional<Entity> findBy(Function<Entity, ? extends X> mapper,
                                           BiPredicate<X, Y> biPredicate,
                                           Y lookupValue) {
        return Optional.ofNullable(lookupValue)
                .flatMap(y -> entities.stream()
                        .filter(Objects::nonNull)
                        .filter(x -> biPredicate.test(mapper.apply(x), y))
                        .findAny());
    }
}

Lookup implementation #2

This relies on a good-ol' Map to perform the lookup.

Advantages

  • LookupUtils can be readily applied to generate other similar lookup Maps.
  • Using LookupUtils can make the lookup more defensive than the first approach (see below)

 

public final class AnotherEntityContainer implements Container {

    private final Set<Entity> entities;
    private final Map<String, Entity> byNames;
    private final Map<String, Entity> byAliases;

    public AnotherEntityContainer(Entity... entities) {
        if (entities == null || entities.length == 0) {
            throw new IllegalArgumentException("At least one entity expected.");
        }
        this.entities = ImmutableSet.copyOf(entities);
        this.byNames = LookupUtils.mapBy(Entity::getName, entities);
        this.byAliases = LookupUtils.mapByMulti(Entity::getAliases, entities);
    }

    @Override
    public Optional<Entity> findByName(String name) {
        return ofNullable(byNames.get(name));
    }

    @Override
    public Optional<Entity> findByAlias(String alias) {
        return ofNullable(byAliases.get(alias));
    }
}

Implementation of LookupUtils

public final class LookupUtils {

    private LookupUtils() {
        // empty
    }

    public static <K, V> Map<K, V> mapBy(Function<? super V, ? extends K> mapper,
                                         V... values) {
        return mapBy(values == null ? empty() : stream(values), mapper, v -> v);

    }

    public static <K, V> Map<K, V> mapByMulti(Function<? super V, ? extends Set<K>>mapper,
                                              V... values) {
        return mapBy(values == null ? Stream.<Entry<K, V>>empty()
                        : stream(values).flatMap(x -> mapper.apply(x).stream().collect(
                                toMap(k -> k, v -> x)).entrySet().stream()),
                Entry::getKey, Entry::getValue);
    }

    private static <T, K, V> Map<K, V> mapBy(Stream<T> stream,
                                         Function<? super T, ? extends K> keyMapper,
                                         Function<? super T, ? extends V> valueMapper) {
        Map<K, V> result = stream.filter(Objects::nonNull).collect(
                collectingAndThen(toMap(keyMapper, valueMapper),
                                    Collections::unmodifiableMap));
        if (result.isEmpty()) {
            throw new IllegalStateException("Empty result map unexpected.");
        }
        if (result.containsKey(null)) {
            throw new IllegalStateException("null keys unexpected.");
        }
        return result;
    }
}

The tests for an empty map or null keys are to make it a requirement that lookup maps shouldn't be empty by definition, and to follow the recommendation from Guava. Implicitly, duplicate keys will also throw an IllegalStateException courtesy of Collectors.toMap(Function, Function).


May I know which would be more preferable in terms of:

  • Readability
  • Maintainability/understanding
  • Performance, for some definition of it (taking in mind 'premature optimization is the root of all evil')
    • Does it matter (i.e. will the answer change) if lookups are performed extensively?
    • Does it matter if there is a million aliases and names, or if there's only hundreds of them?
\$\endgroup\$
2
\$\begingroup\$

The first solution is a lot more compact making it almost automatically a lot easier to comprehend and maintain.

However, performance will be far better with the Map variant. The stream variants won't be able to do any optimization and will need to check the predicate against all elements.

If you already know that this class will become a performance bottleneck (and it could easily become one with a higher number of entities combined with a high number of look-ups) then a more complicated solution is certainly waranted to get acceptable performance.

\$\endgroup\$
2
\$\begingroup\$

I would suggest to process following method:

  1. First implementation should be straight forward without thinking about bottlenecks
  2. After that you may think about unneccessary recalculation and go for enhancements without touching the implemention of 1.

This process applies the Open-Closed-Principle. It helps you do distinguish the different concerns and separating them into separate compilation units. So you will also satisfy the single responsibility principle.

Your first implementation is straight forward. I would go with that and derive a new class that will implement caching concerns you introduced in your second implementation.

The point is: Caches always come with scope and lifetime. These concerns should not flood you straight forward implementation. Caches should be transparent to clients. The interface contract should be the same or enhanced, but never changed or restricted (Liskov substitution principle).

\$\endgroup\$

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.