6
\$\begingroup\$

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;
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Well, setting the value of an instance using another to do it isn't IN GENERAL a bad practice, it depends on the context. I really think it is a hard topic; now, you can use UML diagrams to give you an idea about were are you and what are you doing. Please recur to standard like design instead of doing exotic things if you're learning. \$\endgroup\$ Commented Dec 1, 2020 at 2:25

3 Answers 3

Reset to default
12
\$\begingroup\$

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.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ IllegalStateException, not InvalidStateException. The latter sounds a bit like .NET. \$\endgroup\$ Commented Nov 30, 2020 at 23:11
  • \$\begingroup\$ @Lev M.hey, thanks for a really helpful answer. Would not it be better - instead of creating an init function - to put the code initalizing PdDocument into the getter as @Ralf Kleberhoff is doing with getLastPage()? \$\endgroup\$ Commented Dec 4, 2020 at 22:11
  • 1
    \$\begingroup\$ @krenkz Ralf's version of getLastPage does not initialize anything. It just capitalizes on the fact that we can always get last page from a loaded PDF document, so you don't really need an instance varibale to hold a duplicate of that information. But in which getter would you load the whole PDF document? And why would you want to load it more than once? The idea behind intializer functions is to perform a potentially heavy operation that needs to be done only once in the object's life time. \$\endgroup\$ Commented Dec 4, 2020 at 23:10
  • 1
    \$\begingroup\$ @krenkz it is acceptable to have getters that don't actually retrieve data from instance variables or don't have corresponding setter. A getXXX function can calculate the information based on data provided by nested object, as in the getLastPage example, or some fields may be immutable (not allowed to change after initialization). But as a general rule the only time you initialize anything in a getter, is in the signleton pattern with a static getInstance style function. \$\endgroup\$ Commented Dec 4, 2020 at 23:13
6
\$\begingroup\$

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;
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ If byteArray is expected to change ofter and not the PDDocument not always read it one might consider a lazy evaluation strategy where getPddocument calls PDDocument.load if needed. \$\endgroup\$ Commented Dec 1, 2020 at 9:12
  • 1
    \$\begingroup\$ @Taemyr For that use case, you're right. But then I'd question whether it's useful to create a Pdf instance from the byteArray, if it isn't expected to be used as PDF document, only as byte array. So I supposed the use case to "always" include access to the PDF document. \$\endgroup\$ Commented Dec 1, 2020 at 9:24
  • \$\begingroup\$ +1 Also, since your rewritten class is effectively immutable, it might be useful to document that and to enforce it by making the member variables (and the class itself) final. Immutability can be a useful property, but only if users of your class know about it and can rely on it. \$\endgroup\$ Commented Dec 2, 2020 at 2:29
  • 1
    \$\begingroup\$ The class as written is not effectively immutable. It leaks bytearray from both the constructor and the getter. It may also leak Pddocument, as I'm not aware of whether or not that class is immutable. The Pdf class is also extensible, which means that subclasses may break immutability. At the very least, to be immutable Pdf would need defensive copies of bytearray in both the constructor and the getter, and the class must be made final. \$\endgroup\$ Commented Dec 2, 2020 at 4:49
2
\$\begingroup\$

  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.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.