Best practice to initialize instance variables for a POJO when values are null

Question

I am using Java 8. I haven't found another question that fits mine exactly, and I've come across some conflicting information on best practices with what I'm trying to do.

I'm creating a POJO which is essentially a business object - to be returned by my API - that will have its fields initialized from another object.

I'm using the @Data lombok annotation to generate the getters/setters/toString.

I'm wondering which of the following options would align with best practices:

Note: It is guaranteed that authUser will be not null when calling the User constructor.

1. Null-checks with if-statements and default field values

@Data
public class User {
    private String userId = "";
    private String fullName = "";
    private String email = "";
    private Set<String> roles = Collections.emptySet();

    public User(AuthUser authUser, Set<String> roles) {
        if (authUser.getUserId() != null) {
            this.userId = authUser.getUserId();
        }

        if (authUser.getFullName() != null) {
            this.fullName = authUser.getFullName();
        }

        if (authUser.getEmail() != null) {
            this.email = authUser.getEmail();
        }

        if (roles != null) {
            this.roles = roles;
        }
    }
}

I feel like this is a safe/generic way to do this, and I could store the values retrieved from the getters to local variables so the get calls aren't redundant.

2. Null-checks using ternaries with redundant get calls

@Data
public class User {
    private String userId;
    private String fullName;
    private String email;
    private Set<String> roles;

    public User(AuthUser authUser, Set<String> roles) {
        this.userId = authUser.getUserId() == null ? "" : authUser.getUserId();
        this.fullName = authUser.getFullName() == null ? "" : authUser.getFullName();
        this.email = authUser.getEmail() == null ? "" : authUser.getEmail();
        this.roles = roles == null ? Collections.emptySet() : roles;
    }
}

The purpose of this is to reduce code-cluttering, but I believe Clean Code argues against ternaries, so I'd probably not prefer this option for the sake of readability.

3. Null-checks with Optionals without redundant get calls

@Data
public class User {
    private String userId;
    private String fullName;
    private String email;
    private Set<String> roles;

    public User(AuthUser authUser, Set<String> roles) {
        this.userId = Optional.ofNullable(authUser.getUserId()).orElse("");
        this.fullName = Optional.ofNullable(authUser.getFullName()).orElse("");
        this.email = Optional.ofNullable(authUser.getEmail()).orElse("");
        this.roles = Optional.ofNullable(roles).orElse(Collections.emptySet());
    }
}

The purpose of this is to reduce code-cluttering and redundant get calls without creating local variables in order to remove that redundancy. I think this is preferable to ternaries but am not sure.

4. Null-checks with Apache StringUtils.defaultString and CollectionUtils.emptyIfNull

@Data
public class User {
    private String userId;
    private String fullName;
    private String email;
    private Set<String> roles;

    public User(AuthUser authUser, Set<String> roles) {
        this.userId = StringUtils.defaultString(authUser.getUserId());
        this.fullName = StringUtils.defaultString(authUser.getFullName());
        this.email = StringUtils.defaultString(authUser.getEmail());
        this.roles = CollectionUtils.emptyIfNull(roles);
    }
}

This a little shorter than using Optionals, but I feel like using native Java is preferable to third-party libraries (ApacheUtils is already included as a dependency).

Answer 1

Multiple facets to this answer:

First, direct responses to the given approaches

1.

Initializing fields with a value, only to reinitialize them in a constructor is not useful - skimming the class, which will be done in time, would lead to confusion. When initializing values with defaults, it would be better to modify them using setters. The empty String may, however, not be a good default. A Constructor is supposed to instantiate an Object in a way that it can be used - however, a user without a user id appears relatively useless.

2.

This approach is better, but the concepts are not cleanly understood. First off, Clean Code does not argue against ternaries in general. Ternary operations can be very useful, if utilized correctly. The way you use them here is not a good way to use a ternary. This however is

private static <T> T valueOrDefault(T value, T default) {
  return value != null
    ? value
    : default;
}

This is a ternary which is instantly understandable, as it is entirely alone, and formatted in a way that it makes understanding easier. This is also a suggestion to use here, as declaring this method within the User class allows for the following constructor:

public User(AuthUser authUser, Set<String> roles) {
  this.userId = valueOrDefault(authUser.getUserId(), "");
  // omitted
}

While this is very readable in both constructor and utility method, it is also technically overengineered here, as the use of generics is not necessary in this case, it merely removes the need to declare two extremely similar functions.

3.

Do not use Optional like this. Optional has a very specific and limited use, which is as a return type on specific methods and function calls. Also, while this is one way to remove a redundant get-call, this also adds a rather large amount of unnecessary overhead, plus it is much less readable than the second approach. This reduced readability and increased overhead in comprehension and execution remains even if the formatting in improved for readability.

4.

This is the most acceptable approach in general, however this is only true if the necessary dependencies are already present on the classpath. Adding commons-lang for null-checks only is, while technically adhering to the KISS principle, too much cost for effect.

Second: Considerations

Is this an Object or a data structure? Right now, it is technically speaking both, which is inherently problematic. See here for some explanations, the mentioned book would help you more though.

Considering the construction approach: the constructor seems to copy fields from object a, which is also a kind of user, and append a Set of Strings to the object. So it is neither a copy constructor nor a plain constructor. This is in so far confusing that, when described from a more distanced view, the User is instantiated by passing a User and Roles. The Passed User is not present in the User Object, but some fields of it are. If you find this sentence confusing: people who read your code will most likely think along these lines. At least, I do, as I always try to simplify as much as possible.

As this kind of construction CAN be confusing, it may be useful to create a Mapper or Converter accepting AuthUser and Set<String> and returning User. This would separate the data structure from the logic, and it would allow more validation logic to happen without bloating the user object.

This would also allow the user to be a @Value-class, meaning no setters and all fields final, which makes this object almost immutable and easy to share - almost as the Set still can be modified.

Third: the null-problem

Is null really the same as the empty String in case of id, name and email? And, are "no roles" the same as null? null in java is, despite the dreaded NullPointerException, a very useful value, and replacing it with "I don't want to do null check"-defaults is rarely a good idea.

It would actually be better practice to not modify the values at all and take them at face value, as that allows you to determine if there was an error during authentication or data binding (as data binding errors often lead to empty strings, while authentication errors fail to instantiate objects correctly).

Try to incorporate null-values into your application as valid values - for example as validation values, or as markers for errors.

Also, for good measure, never allow or return null where Collections, Maps, arrays or other similar objects are expected, as effectively no one expects these to be null.