User search implementation

Question

In my Java code I have these three classes. Basically I have a User class, and also a GUI class that holds TextViews and other things (GUI related) for the properties that show on the screen.

The SearchUser is just a object that has the GUI properties. One of these per user object. I noticed that both the user class and the SearchUser class share the same properties, so I took them out and made a BaseUser class, and then extended it to have that.

I don't think the way I structured this is good for Java programming practices. I think some sort of encapsulation or something will be better but I'm not sure what I can do here.

I need the user object and GUI object to also be separate for modularity purposes.

Does anyone see a better way of writing the below code?

BaseUser.java

package sord.object;

public class BaseUser {

    int userid;
    String firstname;
    String lastname;
    String emailaddress;
    int roleId;
    Boolean active;
    String picURL;

}

User.java

package sord.object;

import java.util.Date;
import android.content.Context;
import sord.common.DateTimeFunctions;

public class User extends BaseUser {

    private String password;
    private String phoneNumber;
    private String websiteLink;
    private String statusText;
    private String bioText;
    private Date dateJoined;
    private Date dateLastActive;

    public User(int userid, String firstName, String lastName, 
            String password, String emailAddress, String phoneNumber, 
            String websiteLink, String statusText, String bioText, int roleId, 
            String dateJoined, String dateLastActive, int active, 
            String picURL, Context context) {

        this.userid = userid;
        this.firstname = firstName;
        this.lastname = lastName;
        this.password = password;
        this.emailaddress = emailAddress;
        this.phoneNumber = phoneNumber;
        this.websiteLink = websiteLink;
        this.statusText = statusText;
        this.bioText = bioText;
        this.roleId = roleId;
        this.dateJoined = DateTimeFunctions.ConvertStringToDate(context, dateJoined, false);
        this.dateLastActive = DateTimeFunctions.ConvertStringToDate(context, dateLastActive, true);
        this.active = active == 1;
        this.picURL = picURL;
    }   

    public int getUserId() {
        return userid;
    }

    public String getFirstName() {
        return firstname;
    }

    public String getLastName() {
        return lastname;
    }

    public String getPassword() {
        return password;
    }

    public String getEmailAddress() {
        return emailaddress;
    }

    public String getPhoneNumber() {
        return phoneNumber;
    }

    public String getWebsiteLink() {
        return websiteLink;
    }

    public String getStatusText() {
        return statusText;
    }

    public String getBioText() {
        return bioText;
    }

    public int getRoleId() {
        return roleId;
    }

    public Date getDateJoined() {
        return dateJoined;
    }

    public Date getDateLastActive() {
        return dateLastActive;
    }

    public Boolean getActive() {
        return active;
    }

    public String getPicURL() {
        return picURL;
    }   
}

SearchUser.java

package sord.object;

import java.util.ArrayList;
import android.widget.LinearLayout;
import android.widget.TextView;

public class SearchUser extends BaseUser {

    public Boolean origActive;
    public Boolean selected = false;
    public Boolean deleted = false;
    public Boolean edited = false;

    public TextView nameView;
    public LinearLayout userLayout;

    private ArrayList<PlacementGUI> placementList = new ArrayList<PlacementGUI>();

    public SearchUser(int userid, String firstname, String lastname, 
            String emailaddress, int roleId, int active, String picURL) {

        this.userid = userid;
        this.firstname = firstname;
        this.lastname = lastname;
        this.emailaddress = emailaddress;
        this.roleId = roleId;
        this.active = active == 1;
        this.picURL = picURL;
        this.origActive = active == 1;
    }

    public void AddPlacement(PlacementGUI placement) {
        placementList.add(placement);
    }
}

Answer 1

Designing is art we learn from experience. Object oriented paradigm is "natural" way to explain things.

SearchUser extending BaseUser is wrong naturally, isn't it ? (because SearchUser is not special type of user so can't be subclass) You can have has-a-relation ship there (i.e. SearchUser can have member which is object BaseUser )

BaseUser variables should be private; use methods to access them ( if and when required ).

private ArrayList placementList = new ArrayList<PlacementGUI>

can be rewritten as

private List<PlacementGUI> placementList = new ArrayList<PlacementGUI>

Always program through interface; so later you can change your mind with minimal changes.

Design is something you always do on white paper and change many times.

Answer 2

+1 to @Chirag. Some additional, minor notes:

1. Date objects are mutable. An erroneous (or malicious) client of the class could change its internal state although the class does not provide a setter:

   User user = ...
   user.getDateLastActive().setTime(5);
   

   (Effective Java, 2nd Edition, Item 39: Make defensive copies when needed)

2. The code contains some duplication here:

   this.active = active == 1;
   this.picURL = picURL;
   this.origActive = active == 1;
   

   It could be:

   this.active = active == 1;
   this.picURL = picURL;
   this.origActive = this.active;
   

3. Using a named constants for 1˛in the snippet above would help readers/maintainers because named constanst could express the purpose of value (STATE_ACTIVE), readers might not know what the original author wanted to achieve with the number. Currently it's a magic number.

4. I usually use camelCase for variables like this too:

   this.picURL = picURL;
   

   From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:

   While uppercase may be more common, a strong argument can made in favor of capitalizing only the first letter: even if multiple acronyms occur back-to-back, you can still tell where one word starts and the next word ends. Which class name would you rather see, HTTPURL or HttpUrl?

5. package sord.object;
   

   You could use a better package name. Java Package Names on c2.com

6. A few references for @Chirag's notes:

   • Effective Java, Second Edition, Item 52: Refer to objects by their interfaces
   • Effective Java, Second Edition, Item 16: Favor composition over inheritance
   • Effective Java, Second Edition, Item 13: Minimize the accessibility of classes and members

Answer 3

One of the lessons I came by the hard way is that you really really want to avoid inheritance unless it's absolutely and completely obvious that you need it.

It's really hard to tell what you should do because you didn't put any code in there. The object as a data structure is not a great idea--it's for code.

Honestly in your case everything you have looks like data to me--are you just using it to transfer data back and forth? If so it really doesn't matter much how you structure these fields--just do it in a way that matches your needs (probably one object per screen).

Inheritance should only be used to support code. I've heard it said that the only time you should ever inherit is if you have two children that each override a method in the parent.

Answer 4

Comments and short explanations are in the code comments. Here's an example to demonstrate constructor chaining :

// make this abstract so it's not instantiable
public abstract class BaseUser {

    // limit access to the fields, use accessor & mutator methods instead
    private int userid;
    private String firstname;
    private String lastname;
    private String emailAddress;
    private int roleId;
    private Boolean active;
    private String picURL;

    // initializations of these fields should happen here since BaseUser is the owner of these fields
    public BaseUser(int userid, String firstName, String lastName, 
            String emailAddress, int roleId, int active, String picURL) {
        this.userid = userid;
        this.firstname = firstName;
        this.lastname = lastName;
        this.emailAddress = emailAddress;
        this.roleId = roleId;
        this.active = active == 1;
        this.picURL = picURL;
    }

    ...(other baseUser specific methods to change/read/validate states)
}

public class User extends BaseUser {

    private String password;
    private String phoneNumber;
    private String websiteLink;
    private String statusText;
    private String bioText;
    private Date dateJoined;
    private Date dateLastActive;

    public User(int userid, String firstName, String lastName, 
            String password, String emailAddress, String phoneNumber, 
            String websiteLink, String statusText, String bioText, int roleId, 
            String dateJoined, String dateLastActive, int active, 
            String picURL, Context context) {

        // calling the BaseUser's constructor
        //   this way, there's no duplications on setting these fields in the subtypes of BaseUser
        super(userid, firstName, lastName, emailAddress, roleId, active, picURL);

        // initialize User specific fields
        this.password = password;
        this.phoneNumber = phoneNumber;
        this.websiteLink = websiteLink;
        this.statusText = statusText;
        this.bioText = bioText;
        this.dateJoined = DateTimeFunctions.ConvertStringToDate(context, dateJoined, false);
        this.dateLastActive = DateTimeFunctions.ConvertStringToDate(context, dateLastActive, true);
    }   

    ...(other user specific methods to change/read/validate states)
}   

public class SearchUser extends BaseUser {

    // limit access to the fields, use accessor & mutator methods instead
    private Boolean origActive;
    private Boolean selected = false;
    private Boolean deleted = false;
    private Boolean edited = false;

    private TextView nameView;
    private LinearLayout userLayout;

    private List<PlacementGUI> placementList;

    public SearchUser(int userid, String firstname, String lastname, 
            String emailaddress, int roleId, int active, String picURL) {

        // calling the BaseUser's constructor
        //   this way, there's no duplications on setting these fields in the subtypes of BaseUser
        super(userid, firstName, lastName, emailAddress, roleId, active, picURL);

        // initialize SearchUser specific fields
        this.origActive = active == 1;
        this.placementList = new ArrayList<PlacementGUI>();
    }

    ...(other searchUser specific methods to change/read/validate states)
}

From the design view, if your searchUser isnt actually a user type, it shouldnt extend the BaseUser. Imagine also if in the future searchUser type needs to extend another type, perhaps the BaseSearch type. There are other ways to share the fields through compositions other than doing inheritance.

Example :

// a simple javabean class that can be reused by others
public class BasicUserData {

    private int userid;
    private String firstname;
    private String lastname;
    private String emailAddress;
    private int roleId;
    private Boolean active;
    private String picURL;

    public BasicUserData(int userid, String firstName, String lastName, 
            String emailAddress, int roleId, int active, String picURL) {
        this.userid = userid;
        this.firstname = firstName;
        this.lastname = lastName;
        this.emailAddress = emailAddress;
        this.roleId = roleId;
        this.active = active == 1;
        this.picURL = picURL;
    }

    ...(other basicUserData specific methods to change/read/validate states)
}

public abstract class BaseUser {
    // composition example, embedding another object into another object
    private BasicUserData userData;

    public User(int userid, String firstName, String lastName, 
            String emailAddress, int roleId, int active, String picURL) {
        // initialize the bean
        this.userData = new BasicUserData(userId, firstName, lastName,
                            emailAddress, roleId, active, picUrl);
    }
    public User(BasicUserData userData) {
        this.userData = userData;
    }

    ...(other baseUser specific methods to change/read/validate states)
}

// doesnt need to extend BaseUser anymore
public class SearchUser extends BaseSearch {

    // composition example, embedding another object into another object
    private BasicUserData userData;

    // limit access to the fields, use accessor & mutator methods instead
    private Boolean origActive;
    private Boolean selected = false;
    private Boolean deleted = false;
    private Boolean edited = false;

    private TextView nameView;
    private LinearLayout userLayout;

    private List<PlacementGUI> placementList;

    public SearchUser(int userid, String firstname, String lastname, 
            String emailaddress, int roleId, int active, String picURL) {

        // initialize the bean
        this.userData = new BasicUserData(userId, firstName, lastName,
                            emailAddress, roleId, active, picUrl);

        // initialize SearchUser specific fields
        this.origActive = active == 1;
        this.placementList = new ArrayList<PlacementGUI>();
    }

    ...(other searchUser specific methods to change/read/validate states)
}