Classes for acquiring employee information

Question

I'm looking for a review on the following classes and the comments I applied (class assignment):

package assignment1;

/**
 * Returns a GeneralEmployee object
 * Sets the name, id for the object when called.
 * It increments the id that is assigned to a new employee object so that each employee gets a unique id
 * It can be used to compare if another GeneralEmployee object is equal
 * @author me
 *
 */
public abstract class GeneralEmployee implements Employee {

    private String name;
    private static int initialId = 0;
    private int id;

    /**
     * Sets name to passed name param
     * Increments initialID by 1
     * sets id to value of intialId
     * @param name
     */
    public GeneralEmployee(String name) {
        super();
        this.setName(name);
        initialId++;
        id = Integer.valueOf(initialId);
    }

    /**
     * Returns the name of the emloyee
     * @return
     */
    public String getName() {
        return name;
    }

    /**
     * Sets the name of the employee
     * @param name
     */
    public void setName(String name) {
        this.name = name;
    }

    /**
     * Returns the id of the employee
     * @return
     */
    public int getId() {
        return id;
    }

    /*
     * Returns a String comprising the name, id and pay of the employee
     */
    public String toString() {
        return "name " +  getName() + " id " + getId() + " pay " + getPayCheckAmount();
    }

    /*
     * Returns true if the GeneralEmployee object passed to it is equal to the current GeneralEmployee object
     */
    public boolean equals(Object anOject){
        GeneralEmployee generalEmployee;
        try {
            generalEmployee = (GeneralEmployee) anOject;
        } catch (Exception e) {
            return false;
        }
        return generalEmployee.id==id;
    }

}


package assignment1;

/**
 * This class extends GeneralEmployee
 * It is used to set and get the hoursWorked and calculate payCheckAmount based on hoursWorked 
 * @author me
 *
 */
public class PartTimeEmployee extends GeneralEmployee{

    private double hoursWorked;
    private double hourlyPayRate;

    /**
     * Set name and hourly rate of employee
     * @param name
     * @param hourlyPayRate
     */
    public PartTimeEmployee(String name, double hourlyPayRate) {
        super(name);
        this.hourlyPayRate = hourlyPayRate;             
    }

    /* 
     * Return payCheckAmount by checking if hoursWorked > 40
     * If hoursWorked greater then 40 then calculate regular pay plus overtime pay
     * If hoursWorked is not greater then just calculate regular pay     * 
     * @return payCheckAmount
     */
    @Override
    public double getPayCheckAmount() {

        double payCheckAmount;

        if(hoursWorked > 40){            
            payCheckAmount = (40 * hourlyPayRate) + ((hoursWorked - 40) * (1.5 * hourlyPayRate));
        }
        else{
            payCheckAmount = hoursWorked * hourlyPayRate;
        }

        return payCheckAmount;
    }

    /**
     * Return hoursWorked
     * @return hoursWorked
     */
    public double getHoursWorked() {

        return hoursWorked;
    }

    /**
     * Set hoursWorked
     * @param hoursWorked
     */
    public void setHoursWorked(double hoursWorked){
        this.hoursWorked = this.hoursWorked + hoursWorked;
    }
}


package assignment1;

/**
 * Runnable class that will create 5 employees, setting their name, hourlyPayRate and hoursWorked and to return the payCheckAmount based 
 * on those set values
 * 
 * Also checks to see if any employees have a duplicate id
 * 
 * @author me
 *
 */
public class Driver {

    /**
     * Used to start program
     * @param args
     */
    public static void main(String args[]){

        PartTimeEmployee employee1, employee2,employee3,employee4,employee5;

        // working over 40 hours
        employee1 = new PartTimeEmployee("bob", 5);
        employee1.setHoursWorked(8);        
        employee1.setHoursWorked(8);
        employee1.setHoursWorked(8);
        employee1.setHoursWorked(8);
        employee1.setHoursWorked(10);
        System.out.println(employee1.toString());

        // working under 40 hours
        employee2 = new PartTimeEmployee("cindy", 5.5);
        employee2.setHoursWorked(0);        
        employee2.setHoursWorked(0);
        employee2.setHoursWorked(10);
        employee2.setHoursWorked(10);
        employee2.setHoursWorked(0);
        System.out.println(employee2.toString());   

        // working 40 hours
        employee3 = new PartTimeEmployee("fred", 6);
        employee3.setHoursWorked(8);        
        employee3.setHoursWorked(8);
        employee3.setHoursWorked(8);
        employee3.setHoursWorked(8);
        employee3.setHoursWorked(8);
        System.out.println(employee3.toString());

        // working over 4 hours
        employee4 = new PartTimeEmployee("jim", 6.5);
        employee4.setHoursWorked(8);        
        employee4.setHoursWorked(8);
        employee4.setHoursWorked(8);
        employee4.setHoursWorked(10);
        employee4.setHoursWorked(10);
        System.out.println(employee4.toString());

        // working 0 hours
        employee5 = new PartTimeEmployee("lucy", 7);
        employee5.setHoursWorked(0);        
        employee5.setHoursWorked(0);
        employee5.setHoursWorked(0);
        employee5.setHoursWorked(0);
        employee5.setHoursWorked(0);
        System.out.println(employee5.toString());

        Employee employees[] = new Employee[]{employee1,employee2,employee3,employee4,employee5};

        // call method to check if any of the created employees have a duplicate id
        checkIfEmployeeIdIsDuplicate(employees);

    }

    /**
     * @param employees
     * @return true if a duplicate id is found for PartTimeEmployee
     */
    private static boolean checkIfEmployeeIdIsDuplicate(Employee employees[]){

        if(employees!=null){
          for (int i = 0; i < employees.length; i++) {
              for (int j = 0; j < employees.length; j++) {
                  Object object1 = employees[i];
                  Object object2 = employees[j];
                  if (object1 != null && object2 != null && i!=j) {
                      PartTimeEmployee DuplicateEmployeeId1 = (PartTimeEmployee)object1;
                      PartTimeEmployee DuplicateEmployeeId2 = (PartTimeEmployee)object2;
                      if (DuplicateEmployeeId1.equals(DuplicateEmployeeId2)) {
                          System.out.println(DuplicateEmployeeId1.toString() + " equals name " + DuplicateEmployeeId2.toString());
                      }
                  }
              }
          } 
        }

        return false;

    }

}

package assignment1;

/**
 * interface used for making sure there is a method to return the payCheckAmount
 * @author me
 *
 */
public interface Employee {
    public double getPayCheckAmount();
}

Answer 1

Just a couple of small points:

Constructor: setting fields

The general style of setting fields in a constructor looks like this:

this.name = name;

instead of this.setName(name);. Generally, it is good to use methods once they are defined, but in the case of a constructor, it is better to just set the field directly. If you use a setter, the setter might be overridden by a subclass, and then bugs could be introduced.

Integer valueOf integer

You don't need to call id = Integer.valueOf(initialId);. initialId is already an integer, so just do id = initialId.

@Override

If you override a method, add the @Override annotation, this makes your code easier to read. For example:

@Override
public String toString() {

equals

Use instanceof instead of a try-catch block, it will be faster.

Magic Numbers

Do not hardcode numbers like 40 or 1.5. Store it in a field instead, and give that field a good name. That way, if you need to change the value, you only need to change it in one place, and also the reader will know what eg 40 represents.

checkIfEmployeeIdIsDuplicate: deeply nested & bug

The checkIfEmployeeIdIsDuplicate method never returns true, this seems like a bug.

Also, the method is very deeply nested. To fix this, first of all use an early return like this:

if(employees == null){
    return true; // or throw exception
}

checkIfEmployeeIdIsDuplicate: unnecessary cast & another bug

Why store employees[i] as an Object, and then cast it to PartTimeEmployee? Just store it as PartTimeEmployee directly. Either way, if it is not a PartTimeEmployee, but a different kind of employee, this will cause an error.

Then, there wouldn't really be the need to store it, so your code could look like this:

private static boolean checkIfEmployeeIdIsDuplicate(Employee employees[]) {
    if (employees == null) {
        return true; // or throw exeption
    }
    for (int i = 0; i < employees.length; i++) {
        for (int j = 0; j < employees.length; j++) {
            if (employees[i] != null && employees[j] != null && i != j && employees[i].equals(employees[j])) {
                System.out.println(employees[i].toString() + " equals name " + employees[j].toString());
            }
        }
    }
    return false;
}

It still always returns false though (except if null is passed).

Naming

It is customary to use camelCase in Java, so for example DuplicateEmployeeId1 should be duplicateEmployeeId1. But it is not actually an id, so it should be duplicateEmployee1, or just employee1 (we don't even know yet if it is a duplicate).

Answer 2

You should work on your comments. Why do you say that the class "Returns a GeneralEmployee object"? A method returns something, a class represents it. The comment on the constructor looks quite redundant. What does it add in addition to what is already written in the code?

I think you should try to write self-esplicative code and limit comments to explain the reasons why you did something in that way or something that is particularly obscure. There is no point in repeating what it is already clear from the source code.

Talking about the actual code, do you really need to change the values of name and id? I'd rather make that class immutable, which has a lots of advantage in terms of ease of understanding the code and in term of maintaining it.

Why do you have id = Integer.valueOf(initialId);? Isn't id=initialId equivalent and more readable? I cannot see a reason why you want to do that.

Are you sure that id generation it should be responsibility of the GeneralEmployee class? I think it would be much better to have a separate IdGenerator class, with a NextId() method and call it from outside the constructor and then pass to the constructor of GeneralEmployee the computer identifier. In that way you will have separate classes that address different concerns.

I see you defined equals but not hashCode. In general it is better to define both of them. Here you can find more details.

Answer 3

1. General comment regarding comments

   You should always stick with the Javadoc-style of comments for your methods, because I can see some of them are just multi-line comments. I hope it is just accidental deletes on that extra * on the first line.

2. Employee Interface

   Would you consider adding a method addHoursWorked(double hoursWorked) to your Employee interface, since, they are supposed to be working...? :p

   The reason I suggest using a prefix other than set is because that usually implies (or at least to me) you are over-riding whatever existing values that method is for, which is why this chunk of code was confusing to look at first:

   employee4.setHoursWorked(8);
   employee4.setHoursWorked(8);
   employee4.setHoursWorked(8);
   employee4.setHoursWorked(10);
   employee4.setHoursWorked(10);
   

3. GeneralEmployee abstract class

   In your case, you don't have to call super(). As suggested by @tim, you can simply set id as such:

   id = ++initialId; // prefix ++ to increment *before* assignment to id
   

   The equals() method can be implemented in the simpler more terse form below:

   public boolean equals(Object anOject){
       return anObject instanceof GeneralEmployee && ((GeneralEmployee) anObject).id == id;
   }
   

   Finally, as suggested by @mariosangiorgio, you should over-ride hashCode() too, and I'll leave you with this one weird tip: hashCode() returns an int, your id is both an int and used for testing equivalence in your equals() method, so...

4. PartTimeEmployee implementation

   Inside your getPayCheckAmount(), you can consider using a conditional operator (i.e. ? :) to perform and return the calculation in one step. Without showing you the full code, here's something to start you off with:

   return hoursWorked > 40 ? /* calculate for >40 hrs */ : /* calculate for <=40 hrs */;
   

   Instead of writing this.hoursWorked = this.hoursWorked + hoursWorked in full, you can simply write this.hoursWorked += hoursWorked.

5. Driver class

   You can consider having a method that helps you create a new PartTimeEmployee and add the number of hours worked. This eliminates the code repetition you have currently. For example:

   private static Employee createPartTimeEmployee(final String name, double hourlyPayRate, double... hoursWorked) {
       PartTimeEmployee employee = new PartTimeEmployee(name, hourlyPayRate);
       for (double current : hoursWorked) {
           employee.addHoursWorked(hoursWorked);
       }
       return employee;
   }
   
   // usage
   PartTimeEmployee employee1 = createPartTimeEmployee("bob", 5, 8, 8, 8, 8, 10);
   

   System.out.println() works with Objects too, by calling their toString(). So, you can simply write System.out.println(employee1).

   Finally, are you allowed to use Java Collections classes in your homework assignment, or is that something reserved for the future? If I can use those classes, what I may want to do is to use a HashMap (and this is where over-riding the hashCode() method is relied upon) to store the toString() representations of the employees iterated through, with their ids as the key of the map. If the map already contains an existing value, I know that there has been a duplicate. How that is handled is up to your assignment...