Java - Is this a bad design pattern?

Question

Hi all,

In our application, I have seen code written like this:

User.java (User entity)

public class User
{
  protected String firstName;
  protected String lastName;

 ...
   getters/setters (regular POJO)
}

UserSearchCommand
{
   protected List<User> users;
   protected int currentPage;
   protected int sortColumnIndex;
   protected SortOder sortOrder;

   // the current user we're editing, if at all
   protected User user;

   public String getFirstName()
   {return(user.getFirstName());}

   public String getLastName()
   {return(user.getLastName());}

}

Now, from my experience, this pattern or anti-pattern looks bad to me. For one, we're mixing several concerns together. While they're all user-related, it deviates from typical POJO design. If we're going to go this route, then shouldn't we do this instead?

UserSearchCommand
{
   protected List<User> users;
   protected int currentPage;
   protected int sortColumnIndex;
   protected SortOder sortOrder;

   // the current user we're editing, if at all
   protected User user;

   public User getUser()
   {return(user);}

}

Simply return the user object, and then we can call whatever methods on it as we wish?

Since this is quite different from typical bean development, JSR 303, bean validation doesn't work for this model and we have to write validators for every bean.

Does anyone else see anything wrong with this design pattern or am I just being picky as a developer?

Walter

Answer 1

In returning the user object, you are letting the UserSearchCommand write new information over an existing piece of data, which may not be what one wants to allow as the search should allow for reading the data. Also, what you've done is make someone using the UserSearchCommand have to know the methods/properties/members on the User class which isn't the case in the first implementation.

Answer 2

The Law of Demeter suggests the first example.

Answer 3

While Sjoerd and JB make valid points, depending on your use of the SearchCommand I would make the following argument for the second example. If the operations you are defining in the first example do not effect the behavior of the UserSearchCommand, then by defining getFirstName(), etc you are really just duplicating code, which can lead to maintainability issues, for example what if later you add a middle name to the user class? Then you not only need to add it to user but also an accessor in UserSearchCommand. If doing something to the user would modify the behavior of the search then that could be a valid argument to have the caller access the user through the search command, but this could also be achieved via a mechanism such as PropertyListeners.

As you pointed out the first example is mixing information together and, to me, seems counter-intuitive from and OOP perspective. If it is a matter of restricting access to some properties to the User then it may be a matter of changing the access modifiers, or creating an interface that exposes what you deem safe and an implementation class that exposes appropriate package/protected properties. Unless your UserSearchCommand is a misnomer I would expect it to perform operations involving searching for users and then make those user's available as a unit (not individual properties of a user). That is a search command should perform operations related to a search and a user should contain information about the user.

Of course this is a stylistic question, but I would give my vote to your second example.

Answer 4

I agree with you. I've seen this same technique and fail to see the point: It just means duplicating a whole bunch of code, and for what?

Answer 5

How about a third option using interfaces?

UserSearchCommand
{
  protected List<User> users;
  protected int currentPage;
  protected int sortColumnIndex;
  protected SortOder sortOrder;

  // the current user we're editing, if at all
  protected User user;

  public I_UserNameDetails getUser()
  {
    return((I_UserNameDetails)user);
  }
}

Now you have abstraction through an interface and prevent modification of the user object.