5
\$\begingroup\$

I completed Assignment 2 for MIT OpenCourseWare 6.092, titled "Foo Corp". The code I wrote below calculates a an employee's total pay:

package fooCorp;

class Employee{

    private double payPerHour; //regular hourly wage
    private double overTimePayPerHour; //regular hourly wage * 1.5
    private double basePayHours; //hours worked under regular hourly wage
    private double overTimeHours;//hours worked under overtime pay per hour
    private double totalHours; //adds basePayHours and overTimeHours
    private double totalPay;//base pay + overtime pay



    public void setPayPerHour(double x) //takes parameter 'x' a d sets that equal to this objects payPerHour variable
    {
        this.payPerHour = x;
    }
    public double getpayPerHour() //returns this variables payPerHour variable
    {
        return this.payPerHour;
    }

    public void setOverTimePayPerHour() //calculates and sets overtime payPerHour rate
    {   
            this.overTimePayPerHour = this.payPerHour * 1.5;    
    }
    public double getOverTimePayPerHour() //returns overTimePayPerHour rate
    {
        return this.overTimePayPerHour;
    }


    public void setHours(int x) //sets hours worked into three of this objects variables, basePayHours, overTimeHours and totalHours
    { 
        if(x > 40){
            this.basePayHours = 40;
        }
        else{
            this.basePayHours = x;
        }
        if (x > 40){
            this.overTimeHours = x - 40;
        }
        else{
            this.overTimeHours = 0;
        }

        this.totalHours = this.basePayHours + this.overTimeHours;

    }

    public double getBasePayHours()//returns this object's basePayHours value
    {
        return this.basePayHours; 
    }
    public double getOverTimeHours()//returns this object's overTimeHours value
    {
        return this.overTimeHours; 
    }
    public double getTotalHours()  //returns this object's totalHours value
    {
        return this.totalHours;
    }


    public void calculateTotalPay(){ //calculates this Employee objects total pay and sets it's totalPlay variable to that value
        double basePay = this.getBasePayHours() * this.getpayPerHour();  //calculates basePay
        double overTimePay = this.getOverTimeHours() * this.getOverTimePayPerHour(); //calculates overTimePay
        this.totalPay = basePay + overTimePay;//sets value of this object's totalPay to the sum of basePay and overTimePay
    }
    public double getTotalPay()
    {
        return this.totalPay; //returns this object's totalPay value
    }


}

public class FooCorp {
    public static void main(String[] args)
    {
        Employee employee1 = new Employee();
        employee1.setPayPerHour(8.20);
        employee1.setOverTimePayPerHour();
        employee1.setHours(47);
        employee1.calculateTotalPay();
        System.out.println("You worked  " + employee1.getBasePayHours() + " regular hours."); //should be 40
        System.out.println("Plus "+  employee1.getOverTimeHours() +" hours of overtime."); // should be 7.00
        System.out.println("For a total of " + employee1.getTotalHours() + " hours."); //should be 47
        System.out.println("You get paid $" + employee1.getpayPerHour() + " per hour."); //should be 8.20
        System.out.println("You get paid $" + employee1.getOverTimePayPerHour() + "  per hour for overtime."); // should be 12.3
        System.out.println("Your total pay is: $"+employee1.getTotalPay()); //should be 414.10
    }

}

I feel that this is not proper encapsulation because this seems like a lot of code to do so little? Any advice on something i could do to improve this program would be much appreciated!

\$\endgroup\$
  • \$\begingroup\$ As a simple example, this looks fine to me. Java has always been a pretty verbose language, especially with all the getters and setters. \$\endgroup\$ – Mage Xy Jan 13 '16 at 21:09
  • \$\begingroup\$ Please note that in a real-world system, you should never represent money as doubles due to rounding errors. See: stackoverflow.com/questions/3730019/… \$\endgroup\$ – Eric Stein Jan 14 '16 at 16:09
5
\$\begingroup\$

Department of Redundancy Department

There's a lot of unnecessary variables here, so cleaning that up is going to be a good way to go. Your comments indicate this pretty clearly:

private double payPerHour; //regular hourly wage
private double overTimePayPerHour; //regular hourly wage * 1.5

If overtime pay is always regular pay times 1.5, why do we need it as a separate variable? Just drop the overtime setter and have the getter do the right thing:

public double getOverTimePayPerHour() 
{
    return this.payPerHour * 1.5;
}

Similarly, basePayHours and overTimeHours can be derived from totalHours:

public double getBasePayHours()
{
    return Math.min(this.totalHours, 40);
}

public double getOvertimeHours()
{
    return Math.max(0, this.totalHours - 40);
}

totalPay can be calculated on the fly and doesn't need to be a member variable.

public double getTotalPay()
{
    return getBasePayHours() * this.payPerHour + getOvertimeHours() * this.payPerHour * 1.5;
}

Invalid states

In addition to removing redundancies, the above changes also get rid of another problem in your code - invalid state. getTotalPay() is only valid after you call calculateTotalPay(), which itself is only valid after you call setOverTimePayPerHour(). That's a lot that the user has to keep track of to use your code correctly, and is extremely error-prone.

Excessive commenting

Comments are for making otherwise difficult code clear. Have a complicated algorithm, or doing something that seems unobvious? Comment!

But when your code speaks for itself (which you should always strive for), the comments are, at best, annoying and could easily deteriorate the quality of the code over time if you update the code but forget to the update the comments.

In each of these examples, the comment is literally restating the code:

return this.totalPay; //returns this object's totalPay value
public double getTotalHours()  //returns this object's totalHours value
public double getOverTimePayPerHour() //returns overTimePayPerHour rate
// etc.

On the other hand, the following comment is incorrect:

public void setPayPerHour(double x) //takes parameter 'x' a d sets that equal to this objects payPerHour variable

It doesn't set the parameter, it sets the object's member. Although the comment itself is unnecessary. If setPayPerHour() doesn't set something related to the hourly pay, it's the function's name that's the problem, not the lack of comment.

\$\endgroup\$
2
\$\begingroup\$

Invalid states

To touch on the "invalid states" section mentioned in @Barry's answer, the derivation for overtime rate and getting the calculated pay can be done together with their respective preceding steps:

// rates
public void setPayPerHour(double x) 
{
    this.payPerHour = x;
    this.overTimePayPerHour = getOverTimePayPerHour(x);
}

private double getOverTimePayPerHour(double normalRate) 
{
        return normalRate * 1.5;    
}

// calculation
public double calculateTotalPay()
{
    // return calculated sum from here
}

Commenting

The comments you have at every method can be written in the form of Javadoc too:

/**
 * Calculates the total pay.
 * @return the total pay
 */
public double calculateTotalPay()
{
    // ...
}

Showing output

The bunch of System.out.println() statements you have can alternatively be used in the toString() representation of your Employee class, as it is a form of human-friendly representation of an object's state.

Employee or PayCalculator?

As it stands, there's no state that one usually associates with an employee in your Employee class, such as names. PayCalculator may be a more appropriate name.

Unit testing

System.out.println(... employee1.getBasePayHours() ...); //should be 40
System.out.println(... employee1.getOverTimeHours() ...); // should be 7.00
System.out.println(... employee1.getTotalHours() ...); //should be 47
System.out.println(... employee1.getpayPerHour() ...); //should be 8.20
System.out.println(... employee1.getOverTimePayPerHour() ...); // should be 12.3
System.out.println(... employee1.getTotalPay() ...); //should be 414.10

These checks should be done automatically in the form of unit testing instead of manually by the developer. For example, you can use the TestNG testing framework and Hamcrest matchers to perform the assertions:

@Test
public void testCalculation()
{
    Employee employee1 = new Employee();
    employee1.setPayPerHour(8.20);
    // assuming this is done in setPayPerHour()
    // employee1.setOverTimePayPerHour();
    employee1.setHours(47);
    assertThat(employee1.getBasePayHours(), equalTo(40.0));
    assertThat(employee1.getOverTimeHours(), equalTo(7.0));
    assertThat(employee1.getTotalHours(), equalTo(47.0));
    assertThat(employee1.getpayPerHour(), equalTo(8.2));
    assertThat(employee1.getOverTimePayPerHour(), equalTo(12.3));
    // assuming this is returned from here
    assertThat(employee1.calculateTotalPay()), equalTo(414.1));
}
\$\endgroup\$

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.