Why is my if-else statement not working properly?

Question

I have the double fields hourlyPayRate and hoursWorked.

I am trying to write a constructor and mutator to have specific conditions, those being to not allow a pay rate less than 7.25 or greater than 25. Do not allow hours worked less than 0 or greater than 70. If a value below the minimum is passed, then set the field to the minimum. If a value above the maximum is passed, then set the field to the maximum.

With the code I have, the test for it is passing 7.24, and expecting 7.25 and yet my code is returning 7.24. I cannot see why. The test is also checking my mutator by setting the pay to 8.0, then changing it to 7.25 but my code is returning 8.0 without changing, even though it looks like it should return the 7.25 value. What am I doing wrong?

public class ProductionWorker extends Employee
{   

private double hourlyPayRate;
private double hoursWorked;

public ProductionWorker(String name, String idNumber, 
String hireDate, ShiftType shift, double hourlyPayRate, double hoursWorked)
{
    super(name, idNumber, hireDate, shift);
    this.hourlyPayRate = hourlyPayRate;
    this.hoursWorked = hoursWorked;

    if (hourlyPayRate > 25)
    {
        hourlyPayRate = 25;
    }
    else if (hourlyPayRate < 7.25)
    {
        hourlyPayRate = 7.25;
    }
    else if (hourlyPayRate < 25 && hourlyPayRate > 7.25)
    {
        this.hourlyPayRate = hourlyPayRate;
    }
    else if (hoursWorked < 0)
    {
        hoursWorked = 0;
    }
    else if (hoursWorked > 70)
    {
        hoursWorked = 70;
    }
}
public double getHourlyPayRate()
{
    return hourlyPayRate;
}


public void setHourlyPayRate(double hourlyPayRate)
{

    //this.hourlyPayRate = hourlyPayRate;

    if (hourlyPayRate > 25)
    {
        hourlyPayRate = 25;
    }
    else if (hourlyPayRate < 7.25)
    {
        hourlyPayRate = 7.25;
    }
    else if (hourlyPayRate < 25 && hourlyPayRate > 7.25)
    {
        this.hourlyPayRate = hourlyPayRate;
    }
}


public double getHoursWorked()
{
    return hoursWorked;
}


public void setHoursWorked(double hoursWorked)
{
    this.hoursWorked = hoursWorked;

    if (hoursWorked < 0)
    {
        hoursWorked = 0;
    }
    else if (hoursWorked > 70)
    {
        hoursWorked = 70;
    }
    else if (hoursWorked > 0 && hoursWorked < 70)
    {
        this.hoursWorked = hoursWorked;
    }
 }
}

Also I thought that I was supposed to end if-else statements with just else, but when I do that here the compiler warns me that it isn't a statement?

Answer 1

Why the final keyword saves you countless time:

Make the following changes and you will understand what is wrong and how to avoid it next time.

public ProductionWorker(final String name, 
                        final String idNumber,
                        final String hireDate, 
                        final ShiftType shift, 
                        final double hourlyPayRate, 
                        final double hoursWorked)

If you do the above and use a step-debugger you will understand why the correct reference syntax and logic becomes:

if (hourlyPayRate > 25) { this.hourlyPayRate = 25; }
else if (hourlyPayRate < 7.25) { this.hourlyPayRate = 7.25; }
else { this.hourlyPayRate = hourlyPayRate; }

if (hoursWorked < 0) { this.hoursWorked = 0; }
else if (hoursWorked > 70) { this.hoursWorked = 70; }
else { this.hoursWorked = hoursWorked; }

It is really bad practice to put this much logic in a constructor. Ideally you should have done this validation already in some kind of factory method/object pattern implementation. That said you can reduce the code to one liners that are much more explicit about what they are doing and more maintainable in your specific case.

In this very specific case:

You can reduce the lines of code by expressing them more succinctly.

this.hourlyPayRate = Math.min(Math.max(d,7.25), 25.0);
this.hoursWorked = Math.min(Math.max(d,0.0), 70.0);

Which would not really be onerous to see in a constructor. This is easier to reason about and maintain than a bunch of if/elseif/else statements.

Use a normalizeToRange function, since both are doing the same thing.

Note that the final keyword here is just as important as the private keyword for you to call this method from the constructor.

private final double normalizeToRange(final double min, final double max, final double d)
{
    return d < min ? min : d > max ? max : d;
}

this.hourlyPayRate = this.normalizeToRange(7.25, 25.0, hourlyPayRate);
this.hoursWorked = this.normalizeToRange(0.0, 70.0, hoursWorked);

For Complex If/ElseIf/Else chains:

Here is a simplified example of how to apply a Chain of Responsibility Pattern to eliminate overly long and complex if/elseif/else blocks.

I am sure there will be naysayers that say this is overly complex for this case but I would argue the OP got it wrong enough that this would have let them get it right the first time. This is just a strawman example, illustrating the implementation of the Pattern, The Constraint class could be made more generic and more complex with the default logic for normalize() in a single place. That and how to apply this is an exercise left for the reader.

public class Q43789440
{
    /**
     * Shim/Polyfill for Java < 8 compatibility with Java 8
     *
     * @param <T>
     */
    public interface Predicate<T>
    {
        boolean test(T var1);
    }

    /**
     * Shim/Polyfill for Java < 8 compatibility with Java 8
     *
     * @param <T>
     * @param <F>
     */
    public interface Function<F, T>
    {
        T apply(F var1);

        boolean equals(Object var1);
    }

    public interface Constraint extends Predicate<Double>, Function<Double, Double>
    {
        public Double normalize(final Double input);
    }

    public static class LowerConstraint implements Constraint
    {
        private final Constraint next;

        public LowerConstraint(final Constraint next) { this.next = next; }

        @Override
        public Double normalize(final Double input)
        {
            if (this.test(input)) { return this.apply(input); }
            else { return this.next.normalize(input); }
        }

        @Override
        public boolean test(final Double d)
        {
            return d < 7.25;
        }

        @Override
        public Double apply(final Double d)
        {
            return 7.25;
        }
    }

    public static class UpperConstraint implements Constraint
    {
        private final Constraint next;

        public UpperConstraint(final Constraint next) { this.next = next; }

        @Override
        public Double normalize(final Double input)
        {
            if (this.test(input)) { return this.apply(input); }
            else { return this.next.normalize(input); }
        }

        @Override
        public boolean test(final Double d)
        {
            return d > 25.0;
        }

        @Override
        public Double apply(final Double d)
        {
            return 25.0;
        }
    }

    public static class InRangeConstraint implements Constraint
    {
        @Override
        public Double normalize(final Double input)
        {
            if (this.test(input)) { return this.apply(input); }
            else { throw new IllegalArgumentException(); }
        }

        @Override
        public boolean test(final Double d)
        {
            return d >= 7.25 && d <= 25.0;
        }

        @Override
        public Double apply(final Double d)
        {
            return d;
        }
    }

    public static void main(String[] args)
    {
        final Constraint rules = new LowerConstraint(new UpperConstraint(new InRangeConstraint()));
        for (double d = 0.0; d < 30; d++)
        {
            System.out.printf("%.2f normalized to %.2f", d, rules.normalize(d)).println();
        }
    }
}

Answer 2

Whenever you find yourself re-writing code, you should create a method. This allows you to just call that method, instead of writing the same code-snippets multiple places. There's also some wrong if/else-statement logic.

Let's take a look at how we can improve your code a little bit, which makes it easier to read and debug!

public double hourlyPayRate(double hourlyPayRate) {
    if (hourlyPayRate > 25) {
        hourlyPayRate = 25;
    } else if (hourlyPayRate < 7.25) {
        hourlyPayRate = 7.25;
    }
    return hourlyPayRate;
}

public double hoursWorked(double hoursWorked) {
    if (hoursWorked > 70) {
        hoursWorked = 70;
    } else if (hoursWorked < 0) {
        hoursWorked = 0;
    }
    return hoursWorked;
}

The methods first check whether the input is above a certain value and if so, sets it to that value. Otherwise, it checks to see whether it is below a certain value. Finally, it returns said value. You don't have to check whether it is in-between the lower and upper range at the end since those tests have already been done.

Answer 3

The issue is that in the constructor you set the hourlyPayRate and hoursWorked only once.

this.hourlyPayRate = hourlyPayRate;
this.hoursWorked = hoursWorked;

Here in the if-else statement you are parameter assignment with instance variable assignment. Since the constructor arguments share the same name as the private instance variable. You MUST use the this operator to assign them to the object.

public ProductionWorker(String name, String idNumber, 
    String hireDate, ShiftType shift, double hourlyPayRate, double hoursWorked)

{
    super(name, idNumber, hireDate, shift);
    this.hourlyPayRate = hourlyPayRate;
    this.hoursWorked = hoursWorked;

if (hourlyPayRate > 25)
{
    this.hourlyPayRate = 25;
}
else if (hourlyPayRate < 7.25)
{
    this.hourlyPayRate = 7.25;
}
else if (hourlyPayRate < 25 && hourlyPayRate > 7.25)
{
    this.hourlyPayRate = hourlyPayRate;
}
else if (hoursWorked < 0)
{
    this.hoursWorked = 0;
}
else if (hoursWorked > 70)
{
    this.hoursWorked = 70;
}

The same issue applies in setHoursWorked(double hoursWorked)

public void setHoursWorked(double hoursWorked) { this.hoursWorked = hoursWorked;

if (hoursWorked < 0)
{
    this.hoursWorked = 0;
}
else if (hoursWorked > 70)
{
    this.hoursWorked = 70;
}
else if (hoursWorked > 0 && hoursWorked < 70)
{
    this.hoursWorked = hoursWorked;
}

Answer 4

if (...) {
    doA();
} else if (...) {
    doB();
} else if (...) {
    doC();
}

That's never going to do more than one of doA(), doB(), or doC().

You have two separate constraints on your data: Rate of pay must be limited to a certain range, and hours worked must be within a certain range.

You won't be able to enforce both contstraints with just one if...else if...else if... statement.

Answer 5

First, you need to understand variable scope. In your methods, hourlyPayRate refers to the parameter and this.hourlyPayRate refers to the class instance variable. A value assigned to hourlyPayRate will be lost once you return from the method.

Your setHourlyPayRate mutator should look more like this:

public void setHourlyPayRate(double hourlyPayRate)
{
    if (hourlyPayRate > 25)
    {
        this.hourlyPayRate = 25;
    }
    else if (hourlyPayRate < 7.25)
    {
        this.hourlyPayRate = 7.25;
    }
    else
    {
        this.hourlyPayRate = hourlyPayRate;
    }
}

This assigns the correct value to the class variable based on the value of the parameter. One way to avoid such confusion is to give different names to your internal variables and your parameters.

The second issue with your code is the if .. else if statement in your constructor. The else if part will only be executed if the previous condition was false. In your case, the hoursWorked will only be tested if your hourlyPayRate is exactly 25 or 7.25 since those two exact values are not tested in the previous if statements.