Win a copy of Android Programming: The Big Nerd Ranch Guide this week in the Android forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

Proper Practice when checking a List in a Constructor  RSS feed

 
Sylvia Rosemond
Greenhorn
Posts: 3
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Suppose I have the following Book class(I removed irrelevant code/attributes, because I wanted to focus specially on the List<Author> collection)



Question:

The method checkAuthors checks to see if the list has any items, or if it's null, then it uses a for loop to check each item to make sure it isn't null. 

Is calling a private method from the constructor and doing the work it's doing considered bad practice? If so, how should I correct it?
 
Jeanne Boyarsky
author & internet detective
Marshal
Posts: 36875
481
Eclipse IDE Java VI Editor
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sylvia,
Welcome to CodeRanch!

Calling a private method (or a public final method) is not bad practice. And doing that validation up front is good.

Calling a public non-final method in the constructor is a problem because then a subclass can change behavior you are relying on.
 
Norm Radder
Rancher
Posts: 1904
26
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
A note on variable/method names.  Variable names should be nouns and method names should be verbs.  checkIfEmpty sounds like the name of a method, not a variable.
 
Campbell Ritchie
Sheriff
Posts: 54583
151
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Welcome again

I am not happy about your unmodifiable list call there. I think unmodifiable list is suitable for returning a List but not so good for receiving a List. When you return a List, you have four options (I think four):-
  • 1: Return the List full stop. That means you have two references to it and changes in one place are immediately reflected in the other place. There can be concurrency problems because most Lists aren't threa‍d‑safe.
  • 2: Return an unmodifiable List. That means changes in the original location are reflected in both places but it is impossible to change the List from its destination. If you try to alter the List structurally will cause an exception to be thrown. The destination receives a read‑only copy.
  • 3: Return a copy of the List. That means both source and destination have a List as it was at the time of method completion, but they are independent. Both can be changed, but those changes are not reflected in the other.
  • 4: Copy the List and return it in unmodifiable form. That means that the source can change the List without the destination's List changing at all, but the destination cannot alter the List.
  • Note none of those things stops anybody from taking a fresh copy of the List and modifying that. Nor does anything make any of the elements of the List immutable: you can always change the state of a mutable object.Maybe want to take a copy of that List, so any changes you make to it will be independent of the original. Do the copying before you do the validation.
     
    Sylvia Rosemond
    Greenhorn
    Posts: 3
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Campbell Ritchie wrote:Welcome again

    I am not happy about your unmodifiable list call there. I think unmodifiable list is suitable for returning a List but not so good for receiving a List. When you return a List, you have four options (I think four):-
  • 1: Return the List full stop. That means you have two references to it and changes in one place are immediately reflected in the other place. There can be concurrency problems because most Lists aren't threa‍d‑safe.
  • 2: Return an unmodifiable List. That means changes in the original location are reflected in both places but it is impossible to change the List from its destination. If you try to alter the List structurally will cause an exception to be thrown. The destination receives a read‑only copy.
  • 3: Return a copy of the List. That means both source and destination have a List as it was at the time of method completion, but they are independent. Both can be changed, but those changes are not reflected in the other.
  • 4: Copy the List and return it in unmodifiable form. That means that the source can change the List without the destination's List changing at all, but the destination cannot alter the List.
  • Note none of those things stops anybody from taking a fresh copy of the List and modifying that. Nor does anything make any of the elements of the List immutable: you can always change the state of a mutable object.Maybe want to take a copy of that List, so any changes you make to it will be independent of the original. Do the copying before you do the validation.


    @Campbell Ritchie - Thanks for your reply.
    1. How would I go about making a copy before validation? I could only come up with this, BUT the validation has to occur before the copying or else it creates an error.


                 

    2. The Author object is immutable, so all I have to worry about is the List itself. What would be the best option for receiving a list in the constructor to ensure that it can't be modified after object creation?
    3. Would option 4 be okay in the getter for the list, example:

        


    (Sorry spelling mistakes corrected.)
     
    Junilu Lacar
    Sheriff
    Posts: 10926
    158
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    The main concern you're trying to address with all these gyrations is that of maintaining the integrity of the author list and preserving the encapsulation of the Book object.

    As Campbell was saying, the unmodifiable list would be used in the accessor/getter method for the author list.  For the incoming list being passed to the setter, it's enough to use the ArrayList copy constructor (the one that takes another list whose elements are to be added to the new list).

    It's not good form to do more than one thing in a method like checkAuthors. A method named checkAuthors that returns a new list does not have very good semantics. That's kind of like somebody asking to see your driver's license to make sure you're of legal drinking age (Happy St. Patrick's Day! ) and then handing you back a copy of your driver's license. That would be a little surprising, to say the least, wouldn't it?  What would make more sense is if it returned a boolean true if the list checks out, false otherwise. In that case, the method would probably be better named something like isValid().

    Write code that tells a story:

    Don't let yourself get caught up in the details and lose sight of the big picture. The names checkEmpty and checkIfEmpty don't match the things that they are assigned to. checkEmpty is an Author object while checkIfEmpty is a List of Author objects. That's like calling each of your fingers a "checkDirty" and your hand a "checkIfDirty," which would be kind of silly, right?

    Alternatively, you could have a few helper methods:
     
    Campbell Ritchie
    Sheriff
    Posts: 54583
    151
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Sylvia Rosemond wrote:. . . the validation has to occur before the copying or else it creates an error. . . .
    No, copying shoul‍d precede validation because there are security hazards if validation is done first. It is possible to change the state of an argument after validation and before copying to include naughty code.What sort of error are you getting? If you are getting a null pointer exception, there is nothing wrong with that. It isn't your problem, but the problem of whoever is passing the null reference. The requireNonNull call will produce such an exception in advance. Now all you need to do is verify that the List isn't empty in your verification method. The old version was quite all right.
    . . . What would be the best option for receiving a list in the constructor to ensure that it can't be modified after object creation?
    Are you sure nobody will try to modify the List in your objects? Can you trust yourself not to write add or remove calls on that List? Either what you had before, or what I wrote above. If you are keeping a tight rein on that class and not modifying the List, then does it make any difference?
    3. Would option 4 be okay in the getter for the list, example:. . .
    Yes. But if you are never modifying the List inside your class, any of 2 3 4 will do nicely. Remember that a plain simple copy will allow users to modify the List after receipt, but that is not your problem. Your class will be correct with any of 2 3 or 4.
     
    Campbell Ritchie
    Sheriff
    Posts: 54583
    151
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Junilu Lacar wrote:. . . What would make more sense is if it returned a boolean true if the list checks out, false otherwise. In that case, the method would probably be better named something like isValid(). . . .
    This is where we see there are several ways to skin a cat.
    I might call the method validate or validateArgument, with void return type, and it would throw the Exception proactively. Maybe illegal argument exception, maybe null pointer exception. Remember in my version with requireNonNull, the List reference reaching the validate method won't ever be null, because that has already been tested for. But it is still possible for a null to have been added to that List.

    And now I come to look at Junilu's post properly, I see I am simply repeating his second suggestion.

    An advantage of throwing the exceptions yourself rather than letting the JVM find problems later is that you can create informative messages to go with the exceptions. Also, the sooner you throw exceptions and make people aware of the problem, the less harm it will do and the sooner it can be corrected.
     
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!