6
\$\begingroup\$

I'm making my first steps in OOP and I'm currently doing some exercises. I've done a simple program with class structure like this (I'll provide only essential parts):

Super class:

public abstract class Employee {
    private String lastName;
    private float salary;

    public Employee(String lastName, float salary) {
        this.lastName = lastName;
        this.salary= salary;
    }
}

And several child classes, all similar to this:

public class Worker extends Employee {
    private float hourRate;

    public Worker (String lastName, float salary, float hourRate) {
        super(lastName, salary);
        this.hourRate = hourRate;
    }
}

My concern is, when I'll decide to add some field to the Employee class that have to be set in constructor, let's say firstName, than I have to add it there and in constructor's arguments of all child classes and where I call super() inside them. It doesn't seem to be the best solution... Is there a better way to do such a thing? Maybe there is some pattern I don't know yet?

\$\endgroup\$
5
  • \$\begingroup\$ Is this your actual code or an example having no bearing on reality? \$\endgroup\$
    – Mast
    Commented Jan 17, 2018 at 8:00
  • \$\begingroup\$ These are fragments of my actual code (but it's just academic exercise) \$\endgroup\$
    – Vel
    Commented Jan 17, 2018 at 8:01
  • \$\begingroup\$ Is the exercise solving an actual problem though? Please take a look at our help center. \$\endgroup\$
    – Mast
    Commented Jan 17, 2018 at 8:03
  • \$\begingroup\$ Well as far as I'm concerned it is, I'm doing a project that is meant to be a simple payroll system. And I faced the problem I'm asking about when I started expanding this. \$\endgroup\$
    – Vel
    Commented Jan 17, 2018 at 8:07
  • \$\begingroup\$ Downvoted because the question is currently fuzzy and hypothetical. There could be design patterns and solutions that are alternatives to inheritance, but we can't really advise you properly because we don't know what you need to accomplish. \$\endgroup\$ Commented Jan 17, 2018 at 15:57

4 Answers 4

8
\$\begingroup\$

Short answer: no, there's no better way.

There's also nothing wrong with this way of working. You're adding a field that every instance of this class needs to have for your program to work correctly. That's a good reason to enforce initialising it in the constructor. Your compiler (or even better, your IDE) will tell you where else you need to update your code.


In case you're developing a package that other people use where they need to have some time to adjust to the new version you need to be more careful when making API changes (like adding parameters to a public constructor).

In that case you make the current constructor @deprecated and provide a new one next to it. You can also provide a default value for the new field so that the original constructor still "works". For example:

public class Person {
    private String lastName;
    private String firstName;

    @deprecated
    public Person(String lastName) {
        this("", lastName);
    }

    public Person(String firstName, String lastName){
        this.firstName = firstName;
        this.lastName = lastName;
    }

    ...

This way, their IDE will still tell them they're using an outdated constructor, but at least they'll still be able to run their current code without changing all the calls from the start.


Alternatively you could look into other options besides adding the fields to that class. For example, you could have a lookup class that, given a certain employee, looks up his salary in a table for example. This greatly depends on what your program is supposed to do. With the little information you've given us here it's hard to propose concrete solutions like this.

\$\endgroup\$
3
\$\begingroup\$

There is no "better" way.

  • You can remove them form the Employee constructor and rely on setters
  • You can use a builder
  • You can think of composition instead of extension

But if you want to be sure that all required parameters are given when creating a child. You there is no other way.

\$\endgroup\$
0
\$\begingroup\$

First of all, a bug fix: the properties of Employee that are to be shared with the sub classes should be defined as protected and if they are to be accessed by client classes, they need to be public (or have public getter methods)

now for the answer: you can (and should) have separate setter method for each property. this will allow to create concrete Employee instances and then assign any number of properties after constructor is finished.

public abstract class Employee {
    protected String firstName;
    protected String lastName;
    protected float salary;

    public Employee(String lastName, float salary) {
        setLastName(lastName);  // constructor should call setters as well
        setSalary(salary);      // this is in case setter is doing some logic
    }
    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }
    public void setLastName(String lastName) {
        this.lastName = lastName;
    }
    public void setSalary(float salary) {
        this.salary = salary;
    }
}

public class Worker extends Employee {
...
}

public class EmployeeCreator {

    public void createWorker() {
        Employee worker = new Worker();  // assuming there is an empty constructor 
        worker.setFirstName("John");
        worker.setLastName("Doe");
        worker.setSalary(100.00F);
        ((Worker)worker).setHourRate(10.00F);
    }
}

Now, this puts the responsibility of creating complete valid Employees on the client class. If you want to be able to verify that the new Employees has all mandatory properties at the time of instance creation, you can utilize the Builder pattern and have the build() method validate that all properties were assigned:

public abstract class Employee {
  protected String firstName;
  protected String lastName;
  protected float salary;

  public Employee() {}

  public Employee(String lastName, float salary) {
      setLastName(lastName);  // constructor should call setters as well
      setSalary(salary);      // this is in case setter is doing some logic
  }
  public String getFirstName() {
    return firstName;
  }
  public void setFirstName(String firstName) {
      this.firstName = firstName;
  }
  public String getLastName() {
    return lastName;
  }
  public void setLastName(String lastName) {
      this.lastName = lastName;
  }
  public float getSalary() {
    return salary;
  }
  public void setSalary(float salary) {
      this.salary = salary;
  }
  public boolean isValid() {
    return firstName != null  &&  lastName != null  &&  salary > 0;
  }

  public static final class Builder {
    private Employee newEmployee = null;
    public Builder newWorker() {
        newEmployee = new Worker();
        return this;
    }
    public Builder withFirstName(String firstName) {
        newEmployee.setFirstName(firstName);
        return this;
    }
    public Builder withLastName(String lastName) {
        newEmployee.setLastName(lastName);
        return this;
    }
    public Builder withSalary(float salary) {
        newEmployee.setSalary(salary);
        return this;
    }
    public Builder withHourRate(float hourRate) throws UnsupportedOperationException {
        if (newEmployee instanceof Worker) {
            ((Worker)newEmployee).setHourRate(hourRate);
        } else {
          // cannot call this for any Employee other than Worker
            throw new UnsupportedOperationException();  
        }
        return this;
    }
    public Employee build() throws IllegalStateException {
        if (newEmployee.isValid()) return newEmployee;
          // new enployee not fully assigned
        throw new IllegalStateException();
    }
  }
}

public static class Worker extends Employee {
  ...

 public boolean isValid() {
   return super.isValid() &&  hourRate > 0;
 }
}

public static class EmployeeCreator {
    public void createWorker() {} {
    Employee worker = new Employee.Builder()
            .newWorker()
            .withFirstName("John")
            .withLastName("Doe")
            .withSalary(100.00F)
            .withHourRate(10.00F).build();  // will throw IllegalStateException if instance not assigned correctly
 }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Note that calling overridable method in a constructor is considered a bad practice. stackoverflow.com/questions/18348797/… \$\endgroup\$
    – gervais.b
    Commented Jan 17, 2018 at 12:12
  • \$\begingroup\$ I would argue that in this scenario, the case for calling the setter in the ctor overrides the above-mentioned best practice principle. at the very least, it is worth considering which is more likely to occur: the setter doing some logic that would be missed if not called in the ctor, ot the subclass overriding the setter for a property that is in the super. \$\endgroup\$ Commented Jan 17, 2018 at 12:19
  • 1
    \$\begingroup\$ I would say that if a field is obligated and shouldn't change after construction of the instance, you should not provide a setter. Immutability is a nice thing to have. \$\endgroup\$
    – Imus
    Commented Jan 17, 2018 at 14:49
0
\$\begingroup\$

There is another way. It does not remove the need to do this, but it can significantly reduce it.

Use composition. Instead of Employee containing directly information about person, it can contain object person.

Then adding another field with information about person is no longer change of employee and therefore does not necesitate need to change its descendants.

And Person can enforece invariants about person and Employee will now just enforce that it actually must have Person.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.