Classes without a face
I have a feeling for classes that should not be there. When I saw this cartoon about “how an object-oriented programmer sees the world”, I was struck by the fact that all the names of objects were wrong! These are the names that would be chosen by a poor OO programmer. A good programmer would choose “Door” instead of “IndoorSessionInitializer”. But then the cartoon would not be funny :-)
A similar thing happens in the code base of our current project. Sometimes I see a class that strikes me as odd. Perhaps it has a name that does not communicate; more often it is simply a class that should not exist.
One such example is the UserAuthenticationContainer. WTF?!? you are saying? That’s what I also thought when I saw it. It turns out that a UserAuthenticationContainer contains a hashed password and a User.
class UserAuthenticationContainer {
public final User user;
public final String password;
public UserAuthenticationContainer(User userFrom, String password) {
this.user = userFrom;
this.password = password;
}
}
It was used to authenticate the user:
public User authenticate(String username, String password) {
try {
UserAuthenticationContainer userAuthentication =
registry.getUserAuthenticationContainer(username);
if (!encode(password).equals(userAuthentication.password)) {
throw new WrongPasswordException(username);
}
return userAuthentication.user;
} catch (RecordNotFoundException e) {
throw new UserNotFoundException(username);
}
}
This is unnecessary. This class existed only for the purpose of returning two items from a call. We got rid of it by letting User contain its hashedPassword. Furthermore, we moved the job of matching the user input with the hashed password inside the User object itself. This is how the authentication code looks now:
public User authenticate(String username, String password) {
try {
User user = registry.findByName(username);
if (!user.matchesPassword(password)) {
throw new WrongPasswordException(username);
}
return user;
} catch (RecordNotFoundException e) {
throw new UserNotFoundException(username);
}
}
The matchesPassword method in turn delegates the matching to its PasswordDataItem.
public boolean matchesPassword(String userInput) {
return this.password.matches(userInput);
}
Another example is this FilterByUserShopsCriteria:
public class FilterByUserShopsCriteria implements Criteria {
private final Criteria criteria;
public FilterByUserShopsCriteria(User user, Criteria criteria) {
Collection<String> shopIds
= transform(user.getShops(), toShopIds());
this.criteria = and(criteria, in("ShopID", shopIds));
}
@Override
public String condition() {
return criteria.condition();
}
@Override
public Object[] values() {
return criteria.values();
}
}
Its purpose is to return a Criteria (it represent a Sql “where” condition) that matches the Shops that belong to a user. This class was made unnecessary by moving the generation of that Criteria in the User class itself.
I think that this sort of classes happens when we detach “object-oriented” from “modeling”. An object should be a part of a model of the problem domain. It might also be a “technical” class, like an HtmlForm; this is not part of the core domain of the application, but still has a clearly defined role in the system. When we keep applying OOP in a “syntactical” way, we risk creating objects that may be decoupled, but are not cohesive. Functionality is dispersed far away from the center of responsibility.
The “semantic” clue is that a FilterByUserShopsCriteria is not a natural part of the problem domain. See also this post by Carlo Pescio about how the names we choose for classes have a deep influence on design.
January 5th, 2012 at 09:45
Nice post, Matteo!
Don’t you think that this approach could violate the SRP? Now your user does authentication and generates the Criteria, and what more?
Sometimes I think that one of the problems of OOP is that it could lead to over-bloated objects: reality contains too many things for a class to express!
January 5th, 2012 at 10:05
Thanks Franco!
This is a valid objection. The User could start having too many responsibilities. But I don’t think that an object should really have a *single* responsibility. It’s OK for an object to have a small set of related responsibilities. In my case, the User has the following responsibilities
All of these responsibilities are related to who the User is, so they seem a cohesive set of responsibilities. When you look inside the class, though, you don’t see a whole lot of code. Most of these responsibilities are delegated to data items that the user contains.
January 5th, 2012 at 12:00
Hi Matteo,
this is very interesting. I have a different opinion (as you probably know).
In few word I found the approach “move the auth on the User” unpractical, after a while, no matter what, the entities came up as a blob with too many concerns.
Specifically, if I want to change the way Criteria are used or db is build I don’t want to have to change the User.
So my solution in these cases is using Functional programming approach. FilterByUserShopsCriteria and UserAuthenticationContainer for me they scream to be transformed in pure functions.
In this way also tests are simpler (I don’t want to test criteria in the User tests) and no need for “zombie” classes with a single method.
“zombie” -> you create it, you use it, you kill it, you create it again somewhere else etc.
January 6th, 2012 at 10:14
As usual, design decisions should be tailored to the specific project, so it is entirely possible that in your case a simplified structure, where User takes on a few more responsibilities, was a sound choice.
Generally speaking (and so addressing some of Uberto’s points on authentication being best modeled as a pure function) it might be better to transform the (awful :-) UserAuthenticationContainer class into a Credential class instead.
Actually, in a scaled-up problem domain, you may have a hierarchy of Credential (uid+pwd, biometrics, auth. device, facebook auth., whatever). They have different logic and operate upon different data, therefore a polymorphic, class-based approach looks like a better fit.
I won’t go into the SRP dungeon here, if not to point out that you are *not* (as somewhat implied by comments) moving persistence logic into the User (and not even into the PasswordDataItem, I suppose), at least not as far as authenticate() is concerned, as you delegate that to the registry object (which, I assume, is a sort of Repository).
FilterByUserShopsCriteria (ouch :-)) is more problematic, as we expose a deeper problem here. If I read your code properly, the condition() string will be used later to compose some SQL statement with a “… AND shopId IN ( )” clause.
In more than a few cases, a hand-crafted statement based on a left outer join would perform better (mileage may vary between db engines, table size, etc). That’s the dark side of trying to move away, to some extent, from the underlying storage mechanism (by cutting along some modularity axis which might be at odd with the nature of the underlying mechanism).
About your previous post: I’ve used the hashmap-based approach quite often. Works well for me, and it’s actually a well-known pattern (documented back in 2002 in EJB Design Patterns by Floyd Marinescu as “data transfer hashmap”). In practice, some people hate it unconditionally, and would go through anything to experience the joy of intellisense :-), which requires strongly-typed data transfer objects. I tend to see dynamic objects as a slightly different material, with interesting properties, and like to mix materials according to their response to the actual forces at play.
Keep it up!
Carlo
January 7th, 2012 at 15:41
About the hashmap approach: Uberto would probably be interested to know that it is recommended also in “Functional Programming for Java Developers” by Dean Wampler (http://my.safaribooksonline.com/book/programming/java/9781449312657)
About the possible loss of performance of using a “where” condition when a left outer join might perform better: I might consider thinking about this the day I find I actually have a performance problem. For the moment our performance is fine all around, and the functions are performance-critical do not depend on the speed of authenticating the users in the backoffice app :-)
I realize that, in general, you might have a performance problem if you code too much in the “object” world without thinking about how the database will perform. The most common error is something like this (in Rails):
Transactions.all.filter(|t| t.id == myId)
which pulls every transaction from the database and then selects just one :-)
In fact the part of the system that pulls out transactions for the user interface was implemented with a query optimized from day one for returning paginated data. But apart from places where I know (from experience) that are likely performance problems, coding in a straightforward way is enough. I just keep in mind that every HTTP transactions needs to pull from the database everything that is not in the HTTP request already.
By far, by a very long far, the number one problem I have to fight is against development taking too long, and code getting too tangled. I suspect this is the number one problem for the majority of shops? What do you think?
Correctness is also a concern, but by being very scrupolous in testing the application in various ways we have that more or less under control. Of course TDD is one of the key ingredients, but we also need others (automated ATs for example.)
January 8th, 2012 at 15:42
“About the possible loss of performance of using a “where” condition when a left outer join might perform better: I might consider thinking about this the day I find I actually have a performance problem.”
—
that’s fine of course, but (talking about the approach and not the specific example of user authentication) we have to recognize that exposing criteria that way is a non-modular decision. That is, you expose something from User that somewhere else you combine to form a statement or get objects from a cached table. If you change your decision later on, and move to some ad-hoc sql statements, the entire (entanglement :-) chain has to be somewhat unraveled. Not necessarily a big deal, but in this sense I tend to favor architectures where this tend not to happen.
(I talked a bit about modular decisions here: http://www.carlopescio.com/2010/11/design-structure-and-decisions.html)
Musing a bit with separation of concerns again, while lot of people seem mostly concerned with technological separation (view, model, persistence, etc), I tend to look for clear separation at the domain level as well (or even first). So, assuming I got what you’re doing :-), I would not put knowledge of shops inside the user class. I would rather go the other way around and have shop know user (that would remove the need for the criteria thing too, but again I’m thinking more in sql terms and less in cached objects terms).
By far, by a very long far, the number one problem I have to fight is against development taking too long, and code getting too tangled. I suspect this is the number one problem for the majority of shops? What do you think?
—
While I’m not sure it’s problem #1 everywhere, I’m pretty sure it’s in the top 10 :-))
January 8th, 2012 at 21:23
>About the hashmap approach: Uberto would probably be interested to know that it is recommended also in “Functional Programming for Java Developers” by Dean Wampler
Well of course, after a Lisp session you would use hashmap for everything in Java. :)
Anyway we actually used an hashmap approach (with dedicated hashmap wrappers) in our last project and for sure it helped on the productivity side, but somewhere the code is pretty obscure. So it’s not a way I would suggest always, but we thought that in general we can switch to dedicated intellisensed object later.
January 9th, 2012 at 13:58
@Carlo: the goal is to filter the Transactions that a User might want to see or change. The reason why User generates the Shops filter is that the filter depends on the Privileges that the User has, and on the Shops that the User has access to. As it all depends on information that the User has, it makes sense (it seemed to me) to let the user implement the filter.
I see your point that it might make sense to add the User as a parameter to the methods in the repository, as in
transactionsRepository.findAll(User, Criteria)
but then I would have to move a business rule from a domain object to the repository implementation (how Privileges and Shops interact.) I prefer to keep repositories simple.
January 16th, 2012 at 13:59
More on the HashMap approach:
http://codekata.pragprog.com/2007/01/kata_ten_hashes.html