Setters dependent on other instance variables in Java

Question

I am relatively new to java and I am trying to set up a class with non-trivial setters, but I wonder if setting an instance variable via another one is allowed/good practice?

public class Pdf {

    private byte[] bytearray;
    private String name;
    private PDDocument pddocument;
    private int lastPage;

    public Pdf() {
        super();
    }

    public Pdf(String name, byte[] bytearray) {
        super();
        this.name = name;
        this.bytearray = bytearray;
    }

    public String getName(String name) {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public byte[] getBytearray() {
        return bytearray;
    }

    public void setBytearray(byte[] bytearray) {
        this.bytearray = bytearray;
    }

    public PDDocument getPddocument() {
        return pddocument;
    }

    public void setPddocument() {
        
        PDDocument pddocument = null;
        try {
            pddocument = org.apache.pdfbox.pdmodel.PDDocument.load(this.bytearray);
        } catch (IOException e) {
            e.printStackTrace();
        }   
        this.pddocument = pddocument;
    }

    public int getLastPage() {
        return lastPage;
    }

    public void setLastPage() {
        
        this.lastPage = this.pddocument.getNumberOfPages()-1;
    }
}

Answer 1

Allowed?

Yes. There seems to be nothing technically wrong with your code.

Good practice? No.

Specifically, in your example code setPddocument is not actually a setter.

It is a load / initialize function, that does not set a value in the object, but instead performs an operation under the assumption a certain value (bytearray) has already been set.

It would be better off named init or load, and you probably should set up some code to check if there is a valid array, throwing a different exception in that case. (Like InvalidStateException).

setLastPage is also more of an initializer then a setter.

There, you have no error checking at all, so if it is called before setPddocument your program will crash.

While it is possible to have non-trivial setters that modify several instance variables, the general rule of thumb is: if it does not receive a value from outside to do its job, it is not a setter.

Answer 2

In addition to @notescrew's answer:

Look at your Pdf class from a user's point of view.

I'd like to write

Pdf pdf = new Pdf("MyDoc", bytes);
PDDocument doc = pdf.getPddocument();
int lastPage = pdf.getLastPage();

Your current version forces me to insert some strange setter calls:

Pdf pdf = new Pdf("MyDoc", bytes);
pfd.setPddocument();
PDDocument doc = pdf.getPddocument();
setLastPage();
int lastPage = pdf.getLastPage();

When I've created an object and supplied every necessary information (my preferred way: through your two-argument constructor), I want to be able to use all methods and get consistent results.

For the rare cases where you really want to have an externally-visible initialization, make sure to keep track of your instance's state and throw an IllegalStateException if your user is requesting something depending on a not-yet-done initialization. But that introduces a complexity of its own, so in 99% of cases, it's better to have classes do the necessary initializations themself.

My version of your class would be:

public class Pdf {

    private byte[] bytearray;
    private String name;
    private PDDocument pddocument;

    public Pdf(String name, byte[] bytearray) {
        super();
        this.name = name;
        this.bytearray = bytearray;
        this.pddocument = PDDocument.load(bytearray);
    }

    public String getName() {
        return name;
    }

    public byte[] getBytearray() {
        return bytearray;
    }

    public PDDocument getPddocument() {
        return pddocument;
    }

    public int getLastPage() {
        return pddocument.getNumberOfPages()-1;
    }
}

Answer 3

1. From the code I can see that your Pdf class has no meaning without bytearray. So better way should be like remove the default constructor from the class. So that user will always be allowed to create an object of PDF class with parameterized constructor. Resulting he/she will have to pass bytearray always. So you will never land in a situation where it is null or state is invalid.

2. Method setBytearray you can use to change the data and you can throw exception if user pass null in setter. But I would rather say don't have a setter for it and make it immutable class (by marking class final and making all class members immutable). And for every new document create a new Object.

3. Rather change setPddocument to loadPddocument, in-order to make it a relatable name. As method is loading PDF document from the bytearray.

4. On call to getLastPage and getPddocument you can check, if pddocument is null load it first and return (size/object) else return (size/object) from existing loaded pddocument.

5. Variables name should be corrected. Ex. pddocument should be rather pdDocument. And bytearray should be byteArray.

6. Method getName should not have input parameter.