How to skip creating an object if already exists?

[Alexander Visser]

First I had addEmployee() to create employees. The output was:

Type: ENGINEER, id: 1, monthly salary: 1000.0, commission: 0.0, bonus: 0.0, manager: 4.
Type: ENGINEER, id: 2, monthly salary: 1000.0, commission: 0.0, bonus: 0.0, manager: 4.
Type: SALESMAN, id: 3, monthly salary: 1000.0, commission: 100.0, bonus: 0.0, manager: 4.
Type: MANAGER, id: 4, monthly salary: 2000.0, commission: 0.0, bonus: 1000.0.

Because of inheritance I splitted up to addEngineer(), addSalesman() and addManager(). Now the output is:

Type: ENGINEER, id: 1, monthly salary: 1000.0, manager: 3.
Type: ENGINEER, id: 2, monthly salary: 1000.0, manager: 3.
Type: ENGINEER, id: 1, monthly salary: 1000.0, manager: 3. <-- must actually be skipped
Type: SALESMAN, id: 3, monthly salary: 1000.0, commission: 100.0.
Type: MANAGER, id: 4, monthly salary: 2000.0, bonus: 1000.0.

But the duplicate engineer must actually be skipped if instance already exists. I already wrote an if..else statement.
How can I skip creating an object when an object already exists?

	public boolean addEmployee(int id, Employee.EmployeeType type, double monthlySalary, double commission, double bonus) {
		boolean res = true;
		Employee e = new Employee(id, type, monthlySalary, commission, bonus);
		if (employees.contains(e)) { // Skips well when object already exists.
		        res = false;
		} else {
		        employees.add(e);
		}
		return res;
	}

    public boolean addEngineer(int id, double monthlySalary) {
		boolean res = true;
		Engineer e = new Engineer(id, monthlySalary);
		if (employees.contains(e)) { // Doesn't skip well when object already exists.
		      res = false;
		} else {
		      employees.add(e);
		}
		return res;
    }
    
    public boolean addSalesman(int id, double monthlySalary, double commission) {
		boolean res = true;
		Salesman e = new Salesman(id, monthlySalary, commission);
		if (employees.contains(e)) {
		      res = false;
		} else {
		      employees.add(e);
		}
		return res;
    }
    
    public boolean addManager(int id, double monthlySalary, double bonus) {
		boolean res = true;
		Manager e = new Manager(id, monthlySalary, bonus);
		if (employees.contains(e)) {
		      res = false;
		} else {
		      employees.add(e);
		}
		return res;
    }

[Tim Moores]

What type is "employee"? It looks like Set<Employee> - maybe it should be Map<Integer, Employee> instead. Then you could use the ID for a check of existence.

[Alexander Visser]

Within this class (EmployeesAdmin) is ArrayList<Employee>.

[Tim Moores]

OK, that amounts to the same thing here. Consider a Map instead.

[Junilu Lacar]

Have you implemented equals() for any of these classes? The contains() method will use equals() to determine whether an object is already in the collection or not.

As far as your design is concerned, it's not a very good one. What happens when you want to add additional types? You code will have to be changed to accommodate them, right? That violates a basic design principle: Open Closed Principle It also creates a lot of duplication in your code.

[Junilu Lacar]

If the id attribute establishes the identity of these objects, I would argue that using a Map<Integer, EmployeeType> breaks the encapsulation of these classes. I would prefer using the semantics of equals() instead.

[Alexander Visser]

Yes, I implemented equals() and contains(). It is solved by fixing some errors in the subclasses. Indeed, also the duplicate code is fixed now.

[Campbell Ritchie]

How did you override equals()?