1
\$\begingroup\$

Introduction & Test

I would like to create an object which associates employees to X/Y coordinates. Multiple employees can be associated to each coordinate, but each employee can only be associated with one coordinate.

Also critically: employees must be able to have identical externally-configurable attributes, for example, three employees with the name "Mary".

Here's my test (AssociatorTest.java) that I would like to implement a solution to:

package com.example;

import com.google.common.collect.ImmutableSet;
import org.junit.Test;

import static junit.framework.TestCase.*;

public class AssociatorTest {
    final Employee marySmith = new Employee("Mary");
    final Employee maryJohnson = new Employee("Mary");
    final Employee maryWilliams = new Employee("Mary");
    final Employee maryJones = new Employee("Mary");

    final Coordinates coords1 = new Coordinates(1,2);
    final Coordinates coords2 = new Coordinates(3,4);

    @Test
    public void basicAssociatorValidAssociations() throws EmployeeInTwoSpacesError {
        // valid associations
        Associator associator = new Associator()
                        .associate(marySmith, coords1)
                        .associate(maryJohnson, coords2)
                        .associate(maryWilliams, coords2);

        // lookup coords per employee
        assertEquals(coords1, associator.getLocation(marySmith).get());
        assertEquals(coords2, associator.getLocation(maryJohnson).get());
        assertEquals(coords2, associator.getLocation(maryWilliams).get());

        // Unaffiliated employee has no location
        assertTrue(associator.getLocation(maryJones).isEmpty());

        // lookup employees at coords
        assertEquals(ImmutableSet.of(marySmith), associator.getEmployees(coords1));
        assertEquals(ImmutableSet.of(maryJohnson, maryWilliams), associator.getEmployees(coords2));
        assertEquals(ImmutableSet.of(), associator.getEmployees(new Coordinates(5,6)));
    }

    @Test (expected = EmployeeInTwoSpacesError.class)
    public void tryToAssociateAnEmployeeWithTwoCoords()  throws EmployeeInTwoSpacesError {
        // Employees shouldn't be able to exist in two places at the same time
        new Associator()
                .associate(marySmith, coords1)
                .associate(marySmith, coords2);
    }
}

Implementation

My solution depends on using Guava

Here's my Associator.java:

package com.example;

import com.google.common.collect.*;

import java.util.*;


public class Associator {
    private final ImmutableSetMultimap<Coordinates, Employee> associations;

    public Associator() {
        associations = ImmutableSetMultimap.of();
    }

    private Associator(final Multimap<Coordinates, Employee> associations) {
        this.associations = ImmutableSetMultimap.copyOf(associations);
    }

    public Associator associate(final Employee employee, final Coordinates coordinate)
        throws EmployeeInTwoSpacesError {
        if (associations.containsValue(employee)) {
            throw new EmployeeInTwoSpacesError();
        } else {
            Multimap<Coordinates, Employee> mutableMap = HashMultimap.create(associations);
            mutableMap.put(coordinate, employee);
            return new Associator(mutableMap);
        }
    }

    public Optional<Coordinates> getLocation(final Employee employee) throws EmployeeInTwoSpacesError {
        final Set<Coordinates> found = new HashSet<>();
        for (Map.Entry<Coordinates, Employee> entry : associations.entries()) {
            if (entry.getValue().equals(employee)) {
                found.add(entry.getKey());
            }
        }

        int numLocations = found.size();
        if (numLocations == 1) {
            return Optional.of(found.iterator().next());
        } else if (numLocations > 1) {
            // This should be unnecessary, but I'm being paranoid
            throw new EmployeeInTwoSpacesError();
        } else {
            return Optional.empty();
        }
    }

    public ImmutableSet<Employee> getEmployees(final Coordinates location) {
        return associations.get(location);
    }
}

Here's my Employee.java:

package com.example;

import java.util.Objects;
import java.util.UUID;

public class Employee {
    private final String name;
    private final UUID uuid;

    public Employee(String name) {
        this.name = name;
        this.uuid = UUID.randomUUID();
    }

    public String getName() {
        return name;
    }

    @Override
    public boolean equals(Object o) {
        if (o == this) return true;
        if (!(o instanceof Employee)) {
            return false;
        }
        Employee other = (Employee) o;
        return name.equals(other.name) && uuid.equals(other.uuid);
    }

    @Override
    public int hashCode() {
        return Objects.hash(name, uuid);
    }
}

Here's my Coordinates.java

package com.example;

import java.util.Objects;

public class Coordinates {
    private final int x;
    private final int y;

    public Coordinates(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }

    @Override
    public boolean equals(Object o) {
        if (o == this) return true;
        if (!(o instanceof Coordinates)) {
            return false;
        }
        Coordinates other = (Coordinates) o;
        return x == other.x && y == other.y;
    }

    @Override
    public int hashCode() {
        return Objects.hash(x,y);
    }
}

And for completeness, here's my EmployeeInTwoSpacesError.java:

package com.example;

public class EmployeeInTwoSpacesError extends Exception { }

Main Questions:

Any feedback would be appreciated, but here's some specific questions:

  • Is the Employee's UUID the best course of action here to avoid the case where you have similar objects? I never hear people discuss UUIDs
  • Is the use of Optionals appropriate?
\$\endgroup\$
6
  • 1
    \$\begingroup\$ I'm not sure I understand why you need the associator class instead of having each employee have their location as a class member... \$\endgroup\$
    – Emily L.
    Mar 9 at 23:09
  • \$\begingroup\$ Wouldn't that be a violation of the Single Responsibility Principle? \$\endgroup\$
    – karobar
    Mar 10 at 0:49
  • 1
    \$\begingroup\$ It's not a violation of the SRP. The employee class has one responsibility, to represent an employee (data and operations), the location is a property of an employee in the same way as name is. \$\endgroup\$
    – Emily L.
    Mar 11 at 8:58
  • 1
    \$\begingroup\$ The goal of SRP, DRY, SOLID etc is to create correct, simple and maintainable code. By creating an associator like this you're increasing complexity and adding more risk for bugs compared to just considering the location to be a property of the employee (like name and title), which is counter to the overall goal of all of these principles. \$\endgroup\$
    – Emily L.
    Mar 11 at 9:02
  • 4
    \$\begingroup\$ So your employees are not allowed to change their name if they get married or have a sex change or whatever? Loss of immutability seems moot as humans are mutable whether you like it or not. If you want to extend location information by adding another dimension, you're looking at it from the wrong direction. A location is a coordinate tuple and should be encapsulated in a class. An employee has a location, and should then have an instance of a location object. You break cohesion of the class by externalising the has a relation and this makes code less readable. \$\endgroup\$
    – Emily L.
    Mar 11 at 20:36
2
\$\begingroup\$

Is the Employee's UUID the best course of action here to avoid the case where you have similar objects? I never hear people discuss UUIDs

Since your Associator relies on comparing Employees, using UUID seems better than using mutatable fields that do not clearly identify one and the same Employee. But let's keep in mind, that your Associator, which manages your Employee-Location relationship depends on Employee#equals.

Is the use of Optionals appropriate?

Yes, there is no coding reason to not use Optionals by providing a non-arbitrary way for null checks. As such, you could add @Test cases with an Employee having no Coordinates, so you can estimate if e.g. if(associator.getLocation(employee).isPresent()) is doing what it's supposed to do.

How to associate any number of employees with a location, but only one location per employee?

There seem to be two major drawbacks with your approach:

  1. Adding a Employee-Location pair to your Associator means you a) copy your immutable collection to a new immutable collection + the new value and b) returning a new Associator(...) as the return value, just to create another ImmutableMap of the newly created MutableMap.

  2. Checking, if one Employee occupies two Locations means the Associator traversing all values of all Coordinates in your map, regardless of a key. This seems to defeat the whole purpose of having a Map key in the first place.

Instead, having a Location field in your employee class, and forcing the setLocation() method to just be callable once handles your "One Location per Employee" nicely.

public class Employee {
    Location location = null;

    public Optional<Location> getLocation() {
        return Optional.ofNullable(this.location);
    }

    public void setLocation(Location loc) {
        if(this.location != null) throw AppropriateRuntimeException();
        this.location = loc;
    }
}

Everything else, like transition operations, depends on your design of Moveable objects in a designated Area. But now, you can now use a normal Multimap to store your "Many Employees per Location" relationship.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks very much for this feedback. I'm curious about what you consider drawback #2. You say that retrieving the Location of an Employee is inefficient, which I agree with. But, using your suggested mutable Location field, wouldn't retrieving all Employees at a given location be just as inefficient because you'd have to iterate through all Employees? To me, this is another reason to use my Associator scheme, because you could further develop the Associator so that it includes two maps, one Location->Employee, and the other Employee UUID->Location. \$\endgroup\$
    – karobar
    Mar 19 at 14:37
  • \$\begingroup\$ Sorry for the poor wording (edited drawback #2). The difference is iterating over all Employees of one Coordinate (proposal) vs iterating over all Employees of all Coordinates, effectively ignoring the chosen data structure. \$\endgroup\$
    – Sorin
    Mar 19 at 16:14

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.