5
\$\begingroup\$

Right now, I have base abstract GenericDAO class to execute CRUD operations with different kind of objects. I have Customer and Employee POJO classes which are used in specific DAO classes that extends GenericDAO class.

Questions:

  • How correct is the current base GenericDao implementation and SpecificDAO classes which extend GenericDAO?
  • Is it a good idea 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 can I implement a search method in the GenericDAOImpl abstract class correctly when the Employee search method requires 2 arguments (EmployeeSpecs and PersonalSpecs) and the Customer search method requires only one argument (PersonalSpecs)?
  • How can I implement specs that I can invoke the following getSpec() method inside Customer or Employee classes receiving all specs claases(PersonalSpecs, EmployeeSpecs if we talk about Employee class) applicable for a certain pojo class?

I've read about varargs. Is it possible to use them with generics that I can put search method inside GenericDAO class and can search by multiple object specs?

If you will find that some other places could be optimized or totally improved, please let me know.

NB! If someone knows the anwers on 2 & 3 question, take me know, please.

GenericDAO interface:

public interface GenericDAO<T> {
    public boolean add(T t);
    public boolean update(int index, T t);
    public boolean remove(int index);
    public T getByIndex(int index);
    public 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> {

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

    @Override
    public boolean add(T t) {
        if (t != null && !objects.contains(t))
            objects.add(t);

        return objects.contains(t);
    }

    @Override
    public boolean update(int index, T t) {
        if (index >= 0 && t != null) {
            if (objects.remove(getByIndex(index))) {
                objects.add(t);
                return true;
            }
        }
        return false;
    }


    @Override
    public boolean remove(int index) {
        Object obj = getByIndex(index);

        if (obj != null && objects.contains(obj))
            objects.remove(obj);

        return !objects.contains(obj);
    }

    @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 {

    // Some kind of specific functionality for employee

    public List<Employee> search(PersonalSpecs personalSpecs, EmployeeSpecs employeeSpecs);
}

EmployeeDAO implemenetation:

public class EmployeeDAOImpl extends GenericDAOImpl<Employee> implements EmployeeDAO {

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

        return getAll();
    }
}

CustomerDAO interface:

public interface CustomerDAO {

    // Some kind of specific functionality for customer

    public List<Customer> search(PersonalSpecs personalSpecs);
}

CustomerDAO implementation:

public class CustomerDAOImpl extends GenericDAOImpl<Customer> implements CustomerDAO {

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

        return getAll();
    }
}

Employee class:

public class Employee {
    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 this.personalSpecs.getName().hashCode() + this.personalSpecs.getSurname().hashCode();
    }

    @Override
    public boolean equals(Object obj) {
        return ((Employee) obj).personalSpecs.getName().equals(this.personalSpecs.getName()) && ((Employee) obj).personalSpecs.getSurname().equals(this.personalSpecs.getSurname());
    }

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

}

Customer class:

public class Customer {
    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 this.personalSpecs.getName().hashCode() + this.personalSpecs.getSurname().hashCode();
    }

    @Override
    public boolean equals(Object obj) {
        return ((Customer) obj).personalSpecs.getName().equals(this.personalSpecs.getName()) && ((Customer) obj).personalSpecs.getSurname().equals(this.personalSpecs.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(Object obj) {
        EmployeeSpecs employeeSpecs = (EmployeeSpecs) obj;
        if (employeeSpecs != null) {
            if ((employeeSpecs.getType() != null && employeeSpecs.getType().equals(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(Object obj, boolean strict) {
        PersonalSpecs personalSpecs = (PersonalSpecs) obj;
        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() != null && personalSpecs.getSex().equals(sex)) ||
                        (personalSpecs.getAge() != 0 && personalSpecs.getAge() == age);
        }
        return false;
    }

    @Override
    public String toString() {
        return "[" +
                "name='" + name + '\'' +
                ", surname='" + surname + '\'' +
                ", sex=" + sex +
                ", age=" + age +
                ']';
    }
}
\$\endgroup\$
  • \$\begingroup\$ I don't quite understand what it is that you need, and how that compares to the code you have at the moment. Does your code produce the expected result? What exactly do you think is bad about your current approach? \$\endgroup\$ – Simon Forsberg Jul 18 '14 at 21:55
  • \$\begingroup\$ I think you are looking for a approach like the one I had used codereview.stackexchange.com/questions/36519/…. Have a look. It might give you some idea. \$\endgroup\$ – Aseem Bansal Jul 19 '14 at 12:24
4
\$\begingroup\$

One generic (pun unintended) suggestion: Use braces { } for your if statements to ensure you do not accidentally introduce bugs, because it makes the presentation clearer.

Minor points:

  1. You can compare enums by ==, e.g. employeeSpecs.getType() == type.
  2. Casting of obj in your matches() method is potentially unsafe and may/will throw ClassCastExceptions if you accidentally pass in the wrong object type.

Major points:

  1. hashCode() computation usually does not simply add the hash codes from multiple fields, especially in your case where a Person with the name and surname John, Doe will have the same hashcode as Doe, John. If you are on Java 7, you can consider Objects.hash(Object... values) instead. In fact, if you look at its implementation, the code goes something like this:

    result = 1;
    for (Object element : values) {
        result = 31 * result + (element == null ? 0 : element.hashCode());
    }
    return result;
    
  2. You may want to re-visit the meaning of the boolean return type from your add() and remove() methods in GenericDaoImpl. Using the add() method for example, what happens when the data store objects already contain the incoming object t? Do you still want the method to return true, i.e. t is already present, or return false, because strictly speaking the add() operation isn't successful as objects is not modified at all? In your current implementation, you can consult the Javadoc for Set.add():

Returns: true if this set did not already contain the specified element

Or more generically speaking, look at Collection.add().

Returns: true if this collection changed as a result of the call

edit:

One clarification regarding my final point above: I'm quoting the Javadocs from the Collections classes primarily because you are using a Set currently for your "persistence". Even if you are going to replace it with a database back-end and JDBC APIs that do not necessarily return boolean datatypes, it's worthwhile to consider what is the most suitable return value you need from your own APIs. Perhaps you discover that most of your use cases are just to check again whether the input object for add and remove are persisted or not, and therefore the original implementation will still make sense (you'll want to document it properly too).

Looking at your new changes...

public boolean remove(int index);
public boolean remove(T t);

These two implementations are pretty much the same, you can consider passing getByIndex(index) within the remove(int index) method to the remove(T t) method to reduce code duplication. :)

I also don't think your getByIndex() implementation is doing what you think... ultimately, there is still one instance of your persisted objects in objects. If you are thinking of returning immutable copies of your Customer or Employee objects, then you actually have a bit more work to do.

I'm not sure what your two new updateSpecs methods are doing in your EmployeeDAO class, care to explain? Actually, it'll be better if you can submit a new question for further significant changes, so that the history and answers for this question can be largely consistent...

Finally, onto your original question about searching for different generic types right within your GenericDAOImpl class: I don't think that's possible, and this is where I hope other Java experts can offer their advice or solution on this...

\$\endgroup\$
  • \$\begingroup\$ Thank you very much, that is valuable marks for me and I have already fixed them. But I am still worry about search() method. How to deal with this specs in GenericDAO, is it possible to generify it somehow? Otherwise, I should implement specific search() method for each EmployeeDAOImpl and CustomerDAOImpl? Do you have any ideas on this point? \$\endgroup\$ – Viktor V. Jul 21 '14 at 19:20
  • \$\begingroup\$ Thanks, and one question concerning immutable objects in getByIndex(). I didn't understand exactly how to realize it. I read about clone() method for this purpose but it seems it isn't good practice to use it. Also I saw that someone just create a new Collection and add all objects from one colletion to another, is it corect way? How it can be achieved the best way? As for updateSpecs, I have add them as a workaround due to I don't see any way to generify specs in GenericDAOImpl. \$\endgroup\$ – Viktor V. Jul 22 '14 at 17:11
  • \$\begingroup\$ However, just to know, it is a good idea to separate PersonalSpecs from Personal(the same thing for Employee class) class at whole? \$\endgroup\$ – Viktor V. Jul 22 '14 at 17:21
  • \$\begingroup\$ Creating a new Collection only allows you to modify the contents of the copy, say by adding, removing or even re-ordering (if supported) elements without affecting the original Collection. However, if you get an element from either Collections, both having the same reference to it, changing the element's state will be reflected in both Collections. Given the latest revision to your question, I think we can continue the review in a new question that contains your changes... :) \$\endgroup\$ – h.j.k. Jul 22 '14 at 23:35
  • \$\begingroup\$ I have continued to review my latest changes -> codereview.stackexchange.com/questions/57755/… \$\endgroup\$ – Viktor V. Jul 23 '14 at 7:12

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.