7
\$\begingroup\$

My project has a change on the database, so that every INSERT-UPDATE-DELETE action is logged with triggers on the DB.

For that they have an extra table created:

Table T_USER is new and have a PK, a discriminator field and some other fields, Table T_APP_USER contains an PK witch is also FK to PK of T_USER.

In some Pojo's I need to show the last_changed_user, this should be normally comes from T_APP_USER but if a manuel intervention on the DB is done this could be an T_USER who is not known in T_APP_USER.

The following code works, but I'm interested in review, possibilities for other solution (user may be empty when it's a internal DB user). For the moment I add the AuditUser in the Pojo cause all the methods of User has been abstracted I can still get to all details of a User.

Abstract user class:

@Entity
@Table(name = "T_USER")
@Inheritance(strategy = InheritanceType.JOINED)
@DiscriminatorColumn(name = "USER_TYPE", discriminatorType = DiscriminatorType.CHAR)
public abstract class AuditUser extends PersistentEntity {

    @Id
    @GeneratedValue(generator = "T_USER_SEQ", strategy = GenerationType.SEQUENCE)
    @SequenceGenerator(name = "T_USER_SEQ", sequenceName = "T_USER_SEQ", allocationSize = 1)
    @Column(name = "user_sid", nullable = false)
    private Integer id;
    @Column(name = "STATUS")
    private char status = 'A';

    /**
     * getter for id.
     *
     * @return the id of the user.
     */
    @Override
    public Integer getId() {
        return this.id;
    }

    /**
     * getter for status. A => Application user. D => Internal DB user.
     *
     * @return the status of the user
     */
    public char getStatus() {
        return status;
    }

    /**
     * setter of the status(discriminator) A => Application user. D => Internal
     * DB user.
     *
     * @param status the Status to set.
     */
    public void setStatus(char status) {
        this.status = status;
    }

    /**
     * getter for account.
     *
     * @return the account of the user
     */
    public abstract String getAccount();

    /**
     * getter for "active status".
     *
     * @return the "active status" of the user
     */
    public abstract Boolean getActive();

    /**
     * Gets the army number.
     *
     * @return the army number
     */
    public abstract String getArmyNumber();

    /**
     * getter for email.
     *
     * @return the email of the user
     */
    public abstract String getEmail();

    /**
     * getter for firstName.
     *
     * @return the firstName of the user
     */
    public abstract String getFirstName();

    /**
     * getter for knowing if user comes from LDAP or not.
     *
     * @return true when user comes from LDAP, false when not
     */
    public abstract Boolean getIsLdapAccount();

    /**
     * Gets the last login.
     *
     * @return the last login
     */
    public abstract Date getLastLogin();

    /**
     * getter for lastName.
     *
     * @return the lastName of the user
     */
    public abstract String getLastName();

    /**
     * getter for office.
     *
     * @return the office of the user
     */
    public abstract String getOffice();

    /**
     * getter for password.
     *
     * @return the password of the user
     */
    public abstract String getPassword();

    /**
     * getter for phone number.
     *
     * @return the phone number of the user
     */
    public abstract String getPhone();

    /**
     * Gets the private email.
     *
     * @return the private email
     */
    public abstract String getPrivateEmail();

    /**
     * getter for Set of roles.
     *
     * @return the Set of roles of the user
     */
    public abstract Set<Role> getRoles();

    /**
     * getter for the rank.
     *
     * @return the rank of the user
     */
    public abstract String getTitle();

    /**
     * getter for the fullName.
     *
     * @return the fullname of the user.
     */
    public abstract String getFullName();

    /**
     * getter for unit.
     *
     * @return the unit of the user
     */
    public abstract String getUnit();

    /**
     * Getter for the userName.
     *
     * @return the username of the user.
     */
    public abstract String getUsername();

    @Override
    public String toString() {
        return "User with account : " + this.getAccount()
                + "\n and lastname : " + this.getLastName()
                + "\n and firstname : " + this.getFirstName()
                + "\n and email : " + this.getEmail()
                + "\n and password : " + this.getPassword()
                + "\n and private email : " + this.getPrivateEmail();
    }
}

The implementation of the normal application user:

@Entity
@Table(name = "T_APP_USER", uniqueConstraints = {
    @UniqueConstraint(columnNames = {"account"})})
@DiscriminatorValue(value = "A")
@PrimaryKeyJoinColumn(name = "USER_SID", referencedColumnName = "USER_SID")
@Cache(usage = CacheConcurrencyStrategy.NONE)
@Cacheable(value = false)
public class User extends AuditUser implements UserDetails {

    /**
     * The Constant serialVersionUID.
     */
    private static final long serialVersionUID = -8728575886998109132L;
    private static final int MAX_LENGHT_ACCOUNT = 30;
    private static final int MAX_LENGHT_DEFAULT = 50;
    private static final int PRIME = 31;
    /**
     * The account.
     */
    @Column(name = "account", nullable = true, unique = true, length = MAX_LENGHT_ACCOUNT)
    private String account;
    /**
     * The active.
     */
    @Column(name = "is_Active", nullable = true)
    private Boolean active = Boolean.TRUE; // Must be "true", otherwise you'll get "User is disabled" at login.
    /**
     * The email.
     */
    @Column(name = "email", nullable = true, length = MAX_LENGHT_DEFAULT)
    private String email;
    /**
     * The first name.
     */
    @Column(name = "first_Name", nullable = false, length = MAX_LENGHT_DEFAULT)
    private String firstName;
    /**
     * The last name.
     */
    @Column(name = "last_Name", nullable = false, length = MAX_LENGHT_DEFAULT)
    private String lastName;
    /**
     * The is ldap account.
     */
    @Column(name = "is_Ldap_Account", nullable = true)
    private Boolean isLdapAccount = false;
    /**
     * The office.
     */
    @Column(name = "office", nullable = true, length = MAX_LENGHT_DEFAULT)
    private String office;
    /**
     * The password.
     */
    @Column(name = "password", nullable = true)
    private String password;
    /**
     * The phone.
     */
    @Column(name = "phone", nullable = true, length = MAX_LENGHT_DEFAULT)
    private String phone;
    /**
     * The roles.
     */
    @ManyToMany(fetch = FetchType.EAGER, targetEntity = Role.class)
    @JoinTable(name = "T_USER_ROLE", joinColumns =
            @JoinColumn(name = "user_sid", referencedColumnName = "user_sid"), inverseJoinColumns =
            @JoinColumn(name = "role_sid", referencedColumnName = "role_sid"))
    private Set<Role> roles = new HashSet<Role>();
    /**
     * The title.
     */
    @Column(name = "title", nullable = true, length = MAX_LENGHT_DEFAULT)
    private String title;
    /**
     * The unit.
     */
    @Column(name = "unit", nullable = true, length = MAX_LENGHT_DEFAULT)
    private String unit;
    /**
     * The private email.
     */
    @Column(name = "PRIVATE_EMAIL", nullable = true, length = MAX_LENGHT_DEFAULT)
    private String privateEmail;
    /**
     * The army number.
     */
    @Column(name = "MATRICULE")
    private String armyNumber;
    /**
     * The last login.
     */
    @Column(name = "LAST_LOGIN_DATE")
    @Temporal(TemporalType.DATE)
    private Date lastLogin;

    /**
     * constructor for class.
     */
    public User() {
        this.roles = new HashSet<Role>();
    }

    /**
     * Instantiates a new user.
     *
     * @param user the user
     */
    public User(final org.springframework.security.core.userdetails.User user) {
        this.account = user.getUsername();
        this.roles = new HashSet<Role>();
        for (final GrantedAuthority gr : user.getAuthorities()) {
            this.roles.add(new Role(gr.getAuthority()));
        }
    }

    /**
     * Add a role to a user.
     *
     * @param role the role
     * @return true if OK, false if not OK.
     */
    public boolean addRole(final Role role) {
        return this.roles.add(role);
    }

    /**
     * CompareTo written on account with an ignore case comparator.
     *
     * @param persistentEntity the entity to compare to
     * @return the integer outcome
     */
    @Override
    public int compareTo(final PersistentEntity persistentEntity) {
        if (this.equals(persistentEntity)) {
            return 0;
        }
        if (persistentEntity == null) {
            return -1;
        }
        if (this.getClass() != persistentEntity.getClass()) {
            return -1;
        }
        final User other = (User) persistentEntity;
        return StringComparator.IGNORE_CASE_STRING_AND_TRIM_COMPARATOR.compare(
                this.getAccount(), other.getAccount());
    }

    /**
     * Equals written on account -ignoring the case and trimming-.
     *
     * @param obj the object to compare with
     * @return the boolean outcome
     */
    @Override
    public boolean equals(final Object obj) {
        if (this == obj) {
            return true;
        }
        if (obj == null) {
            return false;
        }
        if (this.getClass() != obj.getClass()) {
            return false;
        }
        final User other = (User) obj;
        if (this.account == null) {
            if (other.account != null) {
                return false;
            }
        } else if (!this.account.equals(other.account)) {
            return false;
        }
        return true;
    }

    /**
     * getter for account.
     *
     * @return the account of the user
     */
    @Override
    public String getAccount() {
        return this.account;
    }

    /**
     * getter for "active status".
     *
     * @return the "active status" of the user
     */
    @Override
    public Boolean getActive() {
        return this.active;
    }

    /**
     * Gets the army number.
     *
     * @return the army number
     */
    @Override
    public String getArmyNumber() {
        return this.armyNumber;
    }

    /* (non-Javadoc)
     * @see org.springframework.security.core.userdetails.UserDetails#getAuthorities()
     */
    @Override
    public Collection<GrantedAuthority> getAuthorities() {
        final Collection<GrantedAuthority> roles2 = new ArrayList<GrantedAuthority>();
        for (final Role role : this.getRoles()) {
            roles2.add(new SimpleGrantedAuthority(role.getAuthority()));
        }
        return roles2;
    }

    /**
     * getter for email.
     *
     * @return the email of the user
     */
    @Override
    public String getEmail() {
        return this.email;
    }

    /**
     * getter for firstName.
     *
     * @return the firstName of the user
     */
    @Override
    public String getFirstName() {
        return this.firstName;
    }

    /**
     * getter for id.
     *
     * @return the id of the user
     */
    @Override
    public Integer getId() {
        return super.getId();
    }

    /**
     * getter for knowing if user comes from LDAP or not.
     *
     * @return true when user comes from LDAP, false when not
     */
    @Override
    public Boolean getIsLdapAccount() {
        return this.isLdapAccount;
    }

    /**
     * Gets the last login.
     *
     * @return the last login
     */
    @Override
    public Date getLastLogin() {
        return this.lastLogin;
    }

    /**
     * getter for lastName.
     *
     * @return the lastName of the user
     */
    @Override
    public String getLastName() {
        return this.lastName;
    }

    /**
     * getter for office.
     *
     * @return the office of the user
     */
    @Override
    public String getOffice() {
        return this.office;
    }

    /**
     * getter for password.
     *
     * @return the password of the user
     */
    @Override
    public String getPassword() {
        return this.password;
    }

    /**
     * getter for phone number.
     *
     * @return the phone number of the user
     */
    @Override
    public String getPhone() {
        return this.phone;
    }

    /**
     * Gets the private email.
     *
     * @return the private email
     */
    @Override
    public String getPrivateEmail() {
        return this.privateEmail;
    }

    /**
     * getter for Set of roles.
     *
     * @return the Set of roles of the user
     */
    @Override
    public Set<Role> getRoles() {
        return this.roles;
    }

    /**
     * getter for the rank.
     *
     * @return the rank of the user
     */
    @Override
    public String getTitle() {
        return this.title;
    }

    @Override
    public String getFullName() {
        return getLastName() + " " + getFirstName();
    }

    /**
     * getter for unit.
     *
     * @return the unit of the user
     */
    @Override
    public String getUnit() {
        return this.unit;
    }

    /* (non-Javadoc)
     * @see org.springframework.security.core.userdetails.UserDetails#getUsername()
     */
    @Override
    public String getUsername() {
        return this.getAccount();
    }

    /**
     * HashCode written on account -ignoring the case and trimming-.
     *
     * @return the integer outcome
     */
    @Override
    public int hashCode() {
        final int result = PRIME
                + ((this.account == null) ? 0 : this.account.toLowerCase().trim()
                .hashCode());
        return result;
    }

    /* (non-Javadoc)
     * @see org.springframework.security.core.userdetails.UserDetails#isAccountNonExpired()
     */
    @Override
    public boolean isAccountNonExpired() {
        return true;
    }

    /* (non-Javadoc)
     * @see org.springframework.security.core.userdetails.UserDetails#isAccountNonLocked()
     */
    @Override
    public boolean isAccountNonLocked() {
        return true;
    }

    /* (non-Javadoc)
     * @see org.springframework.security.core.userdetails.UserDetails#isCredentialsNonExpired()
     */
    @Override
    public boolean isCredentialsNonExpired() {
        return true;
    }

    /* (non-Javadoc)
     * @see org.springframework.security.core.userdetails.UserDetails#isEnabled()
     */
    @Override
    public boolean isEnabled() {
        return active;
    }

    /**
     * Before persisting, the account of the user is put in lowercase and
     * trimmed.
     */
    @PrePersist
    void prePersist() {
        if (this.account != null) {
            this.account = this.account.toLowerCase().trim();
        } else {
            throw new IllegalArgumentException(
                    "Unable to persist a null-value for user.account");
        }
    }

    /**
     * Remove a role from a user.
     *
     * @param role the role
     * @return true if OK, false if not OK.
     */
    public boolean removeRole(final Role role) {
        return this.roles.remove(role);
    }

    /**
     * setter for account of the user.
     *
     * @param account of the user
     */
    public void setAccount(final String account) {
        this.account = account;
    }

    /**
     * setter for active status of the user.
     *
     * @param active status of the user
     */
    public void setActive(final Boolean active) {
        this.active = active;
    }

    /**
     * Sets the army number.
     *
     * @param armyNumber the new army number
     */
    public void setArmyNumber(final String armyNumber) {
        this.armyNumber = armyNumber;
    }

    /**
     * setter for email of the user.
     *
     * @param email of the user
     */
    public void setEmail(final String email) {
        this.email = email;
    }

    /**
     * setter for firstName of the user.
     *
     * @param firstName of the user
     */
    public void setFirstName(final String firstName) {
        this.firstName = firstName;
    }

    /**
     * setting true when user comes from LDAP, false when not.
     *
     * @param isLdapAccount boolean value to tell when user comes from LDAP or
     * not
     */
    public void setIsLdapAccount(final Boolean isLdapAccount) {
        this.isLdapAccount = isLdapAccount;
    }

    /**
     * Sets the last login.
     */
    public void setLastLogin() {
        this.lastLogin = Calendar.getInstance().getTime();
    }

    /**
     * setter for lastName of the user.
     *
     * @param lastName of the user
     */
    public void setLastName(final String lastName) {
        this.lastName = lastName;
    }

    /**
     * setter for office of the user.
     *
     * @param office of the user
     */
    public void setOffice(final String office) {
        this.office = office;
    }

    /**
     * setter for password of the user.
     *
     * @param password of the user
     */
    public void setPassword(final String password) {
        this.password = password;
    }

    /**
     * setter for phone of the user.
     *
     * @param phone of the user
     */
    public void setPhone(final String phone) {
        this.phone = phone;
    }

    /**
     * Sets the private email.
     *
     * @param privateEmail the new private email
     */
    public void setPrivateEmail(final String privateEmail) {
        this.privateEmail = privateEmail;
    }

    /**
     * setter for Set of roles of the user.
     *
     * @param roles the new roles
     */
    public void setRoles(final Set<Role> roles) {
        this.roles = roles;
    }

    /**
     * setter for the rank of the user.
     *
     * @param title the new title
     */
    public void setTitle(final String title) {
        this.title = title;
    }

    /**
     * setter for unit of the user.
     *
     * @param unit of the user
     */
    public void setUnit(final String unit) {
        this.unit = unit;
    }
}

Implementation of the DbUser:

@Entity
@Table(name = "T_DB_USER")
@DiscriminatorValue("D")
@PrimaryKeyJoinColumn(name = "USER_SID", referencedColumnName = "USER_SID")
public class DbUser extends AuditUser {

    @Column(name = "USER_NAME")
    private String name;

    /**
     * getter for the name
     *
     * @return the name of the database user.
     */
    public String getName() {
        return name;
    }

    /**
     * setter for the name
     *
     * @param name the name to set for the database user.
     */
    public void setName(String name) {
        this.name = name;
    }

    /**
     * getter for account.
     *
     * @return the account of the user
     */
    @Override
    public String getAccount() {
        return name;
    }

    /**
     * getter for "active status".
     *
     * @return the "active status" of the user
     */
    @Override
    public Boolean getActive() {
        return false;
    }

    /**
     * Gets the army number.
     *
     * @return the army number
     */
    @Override
    public String getArmyNumber() {
        return "";
    }

    /**
     * getter for email.
     *
     * @return the email of the user
     */
    @Override
    public String getEmail() {
        return "";
    }

    /**
     * getter for firstName.
     *
     * @return the firstName of the user
     */
    @Override
    public String getFirstName() {
        return "";
    }

    /**
     * getter for knowing if user comes from LDAP or not.
     *
     * @return true when user comes from LDAP, false when not
     */
    @Override
    public Boolean getIsLdapAccount() {
        return false;
    }

    /**
     * Gets the last login.
     *
     * @return the last login
     */
    @Override
    public Date getLastLogin() {
        return new Date();
    }

    /**
     * getter for lastName.
     *
     * @return the lastName of the user
     */
    @Override
    public String getLastName() {
        return "Internal DB user";
    }

    /**
     * getter for office.
     *
     * @return the office of the user
     */
    @Override
    public String getOffice() {
        return "";
    }

    /**
     * getter for password.
     *
     * @return the password of the user
     */
    @Override
    public String getPassword() {
        return "";
    }

    /**
     * getter for phone number.
     *
     * @return the phone number of the user
     */
    @Override
    public String getPhone() {
        return "";
    }

    /**
     * Gets the private email.
     *
     * @return the private email
     */
    @Override
    public String getPrivateEmail() {
        return "";
    }

    /**
     * getter for Set of roles.
     *
     * @return the Set of roles of the user
     */
    @Override
    public Set<Role> getRoles() {
        return new HashSet<Role>();
    }

    /**
     * getter for the rank.
     *
     * @return the rank of the user
     */
    @Override
    public String getTitle() {
        return "";
    }

    /**
     * getter for the fullName.
     *
     * @return the fullname of the user.
     */
    @Override
    public String getFullName() {
        return getLastName();
    }

    /**
     * getter for unit.
     *
     * @return the unit of the user
     */
    @Override
    public String getUnit() {
        return "";
    }

    /**
     * Getter for the userName.
     *
     * @return the username of the user.
     */
    @Override
    public String getUsername() {
        return getLastName();
    }
}
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Your code looks good. I've visited this question multiple times and I couldn't find anything. Design-wise there might be improvements, but the implementation looks very good. The only thing I found was this.

/**
 * Equals written on account -ignoring the case and trimming-.
 *
 * @param obj the object to compare with
 * @return the boolean outcome
 */
@Override
public boolean equals(final Object obj) {
    if (this == obj) {
        return true;
    }
    if (obj == null) {
        return false;
    }
    if (this.getClass() != obj.getClass()) {
        return false;
    }
    final User other = (User) obj;
    if (this.account == null) {
        if (other.account != null) {
            return false;
        }
    } else if (!this.account.equals(other.account)) {
        return false;
    }
    return true;
}

This last bit:

    if (this.account == null) {
        if (other.account != null) {
            return false;
        }
    } else if (!this.account.equals(other.account)) {
        return false;
    }
    return true;

Could be this instead:

    if (this.account == null) {
        return other.account == null;
    } else {
        return this.account.equals(other.account);
    }

It simplifies things.


Additionally, I'd take a look at your comments, not all of them make sense...

/**
 * The is ldap account.
 */
@Column(name = "is_Ldap_Account", nullable = true)
private Boolean isLdapAccount = false;

And you seem to have misspelled your constants.

private static final int MAX_LENGHT_ACCOUNT = 30;
private static final int MAX_LENGHT_DEFAULT = 50;

(LENGHT -> LENGTH)

\$\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.