Base generic DAO optimizations

Question

I have base abstract GenericDAO class that executes CRUD operations with different kind of objects. Customer and Employee POJO classes are used in specific DAO classes that extends GenericDAO class. From my previous related question I have some unresolved problems:

Questions:

• How to realize immutable objects in GenericDAOImpl?
• Is it a good idea or not to separate object(i.e. EmployeeSpecs, PersonalSpecs) properties in separate class where some of them are refereced to specific POJO classes? I did it to use search() method easier passing spec object(not by a certain object property) to it.
• How should I deal with search() method? I should put it inside specific DAO which is extended from GenericDAO as it's now or I can generify search() method in GenericDAO somehow? What will be the best way in my case?
• How can I implement getSpec() method inside Customer or Employee classes which will take the opportunity to receive all specs(PersonalSpecs, EmployeeSpecs if we talk about Employee class) applicable for a certain pojo class?

NB! If you see that some piece of code could be optimized, please take me know.

GenericDAO interface:

public interface GenericDAO<T> {
    boolean add(T t);
    boolean update(int index, T t);
    boolean remove(T t);
    T getByIndex(int index);
    List<T> getAll();

    // I want to add search method here if it's possible to search by any multiple object specs
}

GenericDAO implementation:

public abstract class GenericDAOImpl<T> implements GenericDAO<T> {

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

    private Set<T> objects = new HashSet<T>();

    @Override
    public boolean add(T t) {
        if (t != null && !objects.contains(t)) {
            if (objects.add(t)) {
                Logger.info("Added new " + t.toString());
                return true;
            }
        }

        return false;
    }

    @Override
    public boolean update(int index, T t) {
        if (index >= 0 && t != null) {
            T obj = getByIndex(index);
            if (obj != null) {
                if (objects.remove(obj) && objects.add(t)) {
                    Logger.info("Updated " + obj.toString() + " to " + t.toString());
                    return true;
                }
            }
        }
        return false;
    }

    @Override
    public boolean remove(T t) {
        if (t != null && objects.contains(t)) {
            if (objects.remove(t)) {
                Logger.info("Removed " + t.toString());
                return true;
            }
        }

        return false;
    }

    @Override
    public T getByIndex(int index) {
        if (index >= 0 && index < objects.size()) {
            return new ArrayList<T>(objects).get(index);
        }

        return null;
    }

    @Override
    public List<T> getAll() {
        return new ArrayList<T>(objects);
    }
}

EmployeeDAO interface:

public interface EmployeeDAO {
    List<Employee> search(PersonalSpecs personalSpecs, EmployeeSpecs employeeSpecs, boolean nameComparable);
}

EmployeeDAO implemenetation:

public class EmployeeDAOImpl extends GenericDAOImpl<Employee> implements EmployeeDAO {

    @Override
    public List<Employee> search(PersonalSpecs personalSpecs, EmployeeSpecs employeeSpecs, boolean nameComparable) {
        if (personalSpecs != null || employeeSpecs != null) {
            for (Employee employee : getAll()) {
                if (employee.getPersonalSpecs().matches(personalSpecs, nameComparable) || (!nameComparable && employee.getEmployeeSpecs().matches(employeeSpecs))) {
                    add(employee);
                }
            }
        }

        return getAll();
    }
}

CustomerDAO interface:

public interface CustomerDAO {
    List<Customer> search(PersonalSpecs personalSpecs, boolean nameComparable);
}

CustomerDAO implementation:

public class CustomerDAOImpl extends GenericDAOImpl<Customer> implements CustomerDAO {

    @Override
    public List<Customer> search(PersonalSpecs personalSpecs, boolean nameComparable) {
        if (personalSpecs != null) {
            for (Customer customer : getAll()) {
                if (customer.getPersonalSpecs().matches(personalSpecs, nameComparable)) {
                    add(customer);
                }
            }
        }

        return getAll();
    }
}

StaffManagement class:

This class added to have a single entry to Employee and Customer DAOs. Moreover, I hide all functionality of DAOs thatnks to this class.

public class StaffManagement {

    private CustomerDAOImpl customerDAO;
    private EmployeeDAOImpl employeeDAO;

    public StaffManagement() {
        this.customerDAO = new CustomerDAOImpl();
        this.employeeDAO = new EmployeeDAOImpl();
    }

    public StaffManagement(List<Customer> customers, List<Employee> employees) {
        super();
        addAllCustomers(customers);
        addAllEmployees(employees);
    }

    public boolean addCustomer(Customer customer) {
        return customerDAO.add(customer);
    }

    public boolean addEmployee(Employee employee) {
        return employeeDAO.add(employee);
    }

    private boolean addAllCustomers(List<Customer> customers) {
        if (customers == null) {
            return false;
        }

        for (Customer customer : customers) {
            if (!customerDAO.add(customer)) {
                return false;
            }
        }

        return true;
    }

    private boolean addAllEmployees(List<Employee> employees) {
        if (employees == null) {
            return false;
        }

        for (Employee employee : employees) {
            if (!employeeDAO.add(employee)) {
                return false;
            }
        }

        return true;
    }

    public boolean removeCustomer(Customer customer) {
        return customerDAO.remove(customer);
    }

    public boolean removeEmployee(Employee employee) {
        return employeeDAO.remove(employee);
    }

    public Customer getCustomerByIndex(int index) {
        return customerDAO.getByIndex(index);
    }

    public Employee getEmployeeByIndex(int index) {
        return employeeDAO.getByIndex(index);
    }

    public List<Customer> getAllCustomers() {
        return customerDAO.getAll();
    }

    public List<Employee> getAllEmployees() {
        return employeeDAO.getAll();
    }

    public boolean updateEmployeeSpecs(Employee employee, PersonalSpecs personalSpecs, EmployeeSpecs employeeSpecs) {
        boolean change = false;

        if (employee != null) {

            List<Employee> employees = getAllEmployees();
            if (personalSpecs != null) {
                for (Employee existedEmployee : employees) {
                    if (existedEmployee.getPersonalSpecs().matches(personalSpecs, true)) {
                        return false;
                    }
                }

                removeEmployee(employee);
                employee.setPersonalSpecs(personalSpecs);
                change = addEmployee(employee);
            }

            if (employeeSpecs != null) {
                removeEmployee(employee);
                employee.setEmployeeSpecs(employeeSpecs);
                change = addEmployee(employee);
            }
        }

        return change;
    }

    public boolean updateCustomerSpecs(Customer customer, PersonalSpecs personalSpecs) {
        boolean change = false;

        if (customer != null) {

            List<Customer> customers = getAllCustomers();
            if (personalSpecs != null) {
                for (Customer existedCustomer : customers) {
                    if (existedCustomer.getPersonalSpecs().matches(personalSpecs, true)) {
                        return false;
                    }
                }

                removeCustomer(customer);
                customer.setPersonalSpecs(personalSpecs);
                change = addCustomer(customer);
            }
        }

        return change;
    }

    public List<Customer> searchCustomersBySpec(PersonalSpecs personalSpecs, boolean nameComparable) {
        return customerDAO.search(personalSpecs, nameComparable);
    }

    public List<Employee> searchEmployeesBySpec(PersonalSpecs personalSpecs, EmployeeSpecs employeeSpecs, boolean nameComparable) {
        return employeeDAO.search(personalSpecs, employeeSpecs, nameComparable);
    }
}

Employee class:

public class Employee {

/**
** Is it a good idea or not to separate object properties in separate file to easy search any Employee objects by these specs?
**/

    private PersonalSpecs personalSpecs;
    private EmployeeSpecs employeeSpecs;

    public Employee(PersonalSpecs personalSpecs, EmployeeSpecs employeeSpecs) {
        this.personalSpecs = personalSpecs;
        this.employeeSpecs = employeeSpecs;
    }

    public PersonalSpecs getPersonalSpecs() {
        return personalSpecs;
    }

    public void setPersonalSpecs(PersonalSpecs personalSpecs) {
        this.personalSpecs = personalSpecs;
    }

    public EmployeeSpecs getEmployeeSpecs() {
        return employeeSpecs;
    }

    public void setEmployeeSpecs(EmployeeSpecs employeeSpecs) {
        this.employeeSpecs = employeeSpecs;
    }

    @Override
    public int hashCode() {
        return Objects.hash(this.personalSpecs.getName(), this.personalSpecs.getSurname());
    }

    @Override
    public boolean equals(Object obj) {
        // Not strictly necessary, but often a good optimization
        if (this == obj) {
            return true;
        }

        if (!(obj instanceof Employee)) {
            return false;
        }

        Employee otherEmployee = (Employee) obj;
        return (personalSpecs.getName().equals(otherEmployee.getPersonalSpecs().getName())
                && personalSpecs.getSurname().equals(otherEmployee.getPersonalSpecs().getSurname()));
    }

    @Override
    public String toString() {
        return "Employee:\n" +
                personalSpecs +
                "\n" +
                employeeSpecs;
    }
}

Customer class:

public class Customer {

/**
** Is it a good idea or not to separate object properties in separate file to easy search any Employee objects by these specs?
**/

    private PersonalSpecs personalSpecs;
    private int bonus;

    public Customer(PersonalSpecs personalSpecs) {
        this.personalSpecs = personalSpecs;
    }

    public Customer(PersonalSpecs personalSpecs, int bonus) {
        this.personalSpecs = personalSpecs;
        this.bonus = bonus;
    }

    public PersonalSpecs getPersonalSpecs() {
        return personalSpecs;
    }

    public void setPersonalSpecs(PersonalSpecs personalSpecs) {
        this.personalSpecs = personalSpecs;
    }

    public int getBonus() {
        return bonus;
    }

    public void setBonus(int bonus) {
        this.bonus = bonus;
    }

    @Override
    public int hashCode() {
        return Objects.hash(this.personalSpecs.getName(), this.personalSpecs.getSurname());
    }

    @Override
    public boolean equals(Object obj) {
        // Not strictly necessary, but often a good optimization
        if (this == obj) {
            return true;
        }

        if (!(obj instanceof Customer)) {
            return false;
        }

        Customer otherCustomer = (Customer) obj;
        return (personalSpecs.getName().equals(otherCustomer.getPersonalSpecs().getName())
                && personalSpecs.getSurname().equals(otherCustomer.getPersonalSpecs().getSurname()));
    }

    @Override
    public String toString() {
        return "Customer: " +
                personalSpecs +
                ", bonus=" + bonus;
    }
}

EmployeeSpecs class:

public class EmployeeSpecs {
    private EmployeeType type;
    private String position;
    private Date start;

    public enum EmployeeType {
        CONTRACTOR,
        FULLTIME
    }

    public EmployeeSpecs(EmployeeType type, String position) {
        this.type = type;
        this.position = position;
    }

    public EmployeeType getType() {
        return type;
    }

    public void setType(EmployeeType type) {
        this.type = type;
    }

    public String getPosition() {
        return position;
    }

    public void setPosition(String position) {
        this.position = position;
    }

    public Date getStart() {
        return start;
    }

    public void setStart(Date start) {
        this.start = start;
    }

    public boolean matches(EmployeeSpecs employeeSpecs) {
        if (employeeSpecs != null) {
            if ((employeeSpecs.getType() == type)
                    || (employeeSpecs.getPosition() != null && !employeeSpecs.getPosition().isEmpty() && employeeSpecs.getPosition().equals(position))
                    || (employeeSpecs.getStart() != null && employeeSpecs.getStart().equals(start))) {
                return true;
            }
        }
        return false;
    }

    @Override
    public String toString() {
        return "[" +
                "type=" + type +
                ", position='" + position + '\'' +
                ", start=" + start +
                ']';
    }
}

PersonalSpecs class:

public class PersonalSpecs {

    private String name;
    private String surname;
    private Sex sex;
    private int age;

    public enum Sex {MALE, FEMALE}

    public PersonalSpecs(String name, String surname, Sex sex) {
        this.name = name;
        this.surname = surname;
        this.sex = sex;
    }

    public PersonalSpecs(String name, String surname, Sex sex, int age) {
        this.name = name;
        this.surname = surname;
        this.age = age;
        this.sex = sex;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public String getSurname() {
        return surname;
    }

    public void setSurname(String surname) {
        this.surname = surname;
    }

    public Sex getSex() {
        return sex;
    }

    public void setSex(Sex sex) {
        this.sex = sex;
    }

    public int getAge() {
        return age;
    }

    public void setAge(int age) {
        this.age = age;
    }

    public boolean matches(PersonalSpecs personalSpecs, boolean strict) {
        if (personalSpecs != null) {
            if (strict) {
                // Check matches only by full name
                return (personalSpecs.getName() != null && !personalSpecs.getName().isEmpty() && personalSpecs.getName().equals(name))
                        && (personalSpecs.getSurname() != null && !personalSpecs.getSurname().isEmpty() && personalSpecs.getSurname().equals(surname));
            } else {
                // Check any matches
                return (personalSpecs.getName() != null && !personalSpecs.getName().isEmpty() && personalSpecs.getName().equals(name))
                        || (personalSpecs.getSurname() != null && !personalSpecs.getSurname().isEmpty() && personalSpecs.getSurname().equals(surname))
                        || (personalSpecs.getSex() == sex)
                        || (personalSpecs.getAge() != 0 && personalSpecs.getAge() == age);
            }
        }
        return false;
    }

    @Override
    public String toString() {
        return "[" +
                "name='" + name + '\'' +
                ", surname='" + surname + '\'' +
                ", sex=" + sex +
                ", age=" + age +
                ']';
    }
}

Answer 1

• How to realize immutable objects in GenericDAOImpl?

By creating copies of the objects you get in, rather than keeping references to the objects. With generics this is may be tricky but if T extends Cloneable you could at least call clone() on it to create a field for field copy. This will not get you the control you would have if you knew which types you use but it is something at least. As output (when asked for a property) you should again make a copy, so that noone can alter your internal state. basic rule of thumb: don't trust input you get to be unmutated, don't trust output you give to stay unmutated.

Your employee DAO maintains an internal set of objects (which is fine for an in-memory version of a dao I suppose. though it's not "accessing" anything but it's internal map/list/set. ) However you seem to be mixing this up in your search. On a dao, I would expect a search to return a list of objects without changing it's internal state.

However in your case, if I do: (excuse the end of line comments)

EmployeeDao dao = (get from somewhere);
dao.add(someEmployee);
dao.add(otherEmployee);
dao.getAll(); // returns someEmployee, otherEmployee;

// assume spec matches only otherEmployee
// search returns someEmployee, otherEmployee
dao.search(specX, specY);  // returns someEmployee, otherEmployee

I might be misunderstanding the above code ofcourse, but I think your search should do:

public List<Employee> search(PersonalSpecs personalSpecs, EmployeeSpecs employeeSpecs, boolean nameComparable) {
    List<Employee> result = new ArrayList<Employee>();

    if (personalSpecs == null && employeeSpecs == null) {
        return result;
    }

    // there is actually something to search on.
    for (Employee employee : getAll()) {
        if (employee.getPersonalSpecs().matches(personalSpecs, nameComparable) || (!nameComparable && employee.getEmployeeSpecs().matches(employeeSpecs))) {
            result.add(employee);
        }
    }

    return result;
}

ofcourse in combination with clone : result.add(employee.clone())

• Is it a good idea or not to separate object(i.e. EmployeeSpecs, PersonalSpecs) properties in separate class where some of them are refereced to specific POJO classes? I did it to use search() method easier passing spec object(not by a certain object property) to it.

I think you are better off keeping your properties divided by how your domain is organized rather than how a search happens to need them grouped. This might be the same at the moment but it may change. What if you have 3 more types of searches that work on overlapping objects. However you could have the object return a representation of itself if you want to do it that way. This will allow an object to have all sorts of properties organized in a number of ways, but when you need a spec version of a set of attributes it can create this.

// in Employee: 
public final EmployeeSpec getEmployeeSpec() {
    EmployeeSpec spec = new EmployeeSpec(this.xx, this.yy);
}

This still limits you to some degree. (it ties you to the spec, you have to add it to every thing you would like to search for). I'm getting a little out of my depth here, but you could have a "Spec" class that does something like:

// could call it specmatcher but I didn't want to make it look like it was still using spec.
public interface Matcher<T> {
     public boolean matches(T match);  
}

// ofcourse implement this as you wish, in your case with strict/nonstrict.
public static class EmployeeMatcher extends Matcher<Employee> {
     private EmployeeType type;
     public boolean matches(Employee match) {
          if (type.equals(match.getType()) {
               return true;
          }
     }
}

You would need to alter your search method to do:

 public List<Employee> search(Matcher<Employee> matcher) {
    List<Employee> result = new ArrayList<Employee>();
    for (Employee employee : getAll()) {
        if (matcher.matches(employee) {
            result.add(employee);
        }
    }
    return result;
}

• How should I deal with search() method? I should put it inside specific DAO which is extended from GenericDAO as it's now or I can generify search() method in GenericDAO somehow? What will be the best way in my case?

The below code should do that (replace Employee with T)

public List<T> search(Matcher<T> matcher) {
    List<T> result = new ArrayList<T>();
    for (T t : getAll()) {
        if (matcher.matches(t) {
            result.add(t);
        }
    }
    return result;
}

• How can I implement getSpec() method inside Customer or Employee classes which will take the opportunity to receive all specs(PersonalSpecs, EmployeeSpecs if we talk about Employee class) applicable for a certain pojo class?

I would drop specs altogether and use matcher. or keep specs at least only as ways to feed matchers.

Again (mentioned it earlier) part of this is an educated guess. I'm hoping I've understood your question.

Pointers (and please if anyone disagrees, leave me a comment):

• search and similar lookup methods should not mutate your internal state. work on the stack or fill in an object you receive as a parameter instead.

• If you do need to have a strict and a nonstrict version of a method, I would personally not use this:

  public boolean matches(PersonalSpecs personalSpecs, boolean strict)

but I would use:

public boolean matchesLoose(PersonalSpecs personalSpecs);
public boolean matchesStrict(PersonalSpecs personalSpecs);    

this will make your internal logic simpler and will reduce the cyclomatic complexity. Ofcourse this doesn't always go, but booleans that split between 2 entirely separate code paths don't really add anything. ofcourse if you want 1 entrypoint you could have

public boolean matches(PersonalSpecs personalSpecs, boolean strict) {
    if (strict) {
         return matchesStrict(personalSpecs);
    } else {
         return matchesLoose(personalSpecs);
    }
}

public boolean matchesLoose(PersonalSpecs personalSpecs);
public boolean matchesStrict(PersonalSpecs personalSpecs);