题
We have multiple pages searching for users, each site having different search parameters. Sometimes, we have 2 parameters, sometimes 4 and most of these parameters overlap. So we have kind of (simplified) this code:
SearchUser()
{
// Get values from somewhere
Service.SearchActiveUser(firstname, lastname);
}
SearchUserSomewhereElse()
{
// Get values from somewhere
Service.SearchUser(lastname, id);
}
// some other layer
SearchActiveUser(string firstname, string lastname)
{
var users = from user in allusers
where user.IsActive == true
select user;
if (!String.IsNullOrEmpty(firstname))
users = users.Where(user => user.Firstname == firstname);
if (!String.IsNullOrEmpty(lastname))
users = users.Where(user => user.Lastname == lastname);
return users
}
SearchUser(string lastname, int? id)
{
var users = from user in allusers
select user;
if (!String.IsNullOrEmpty(lastname))
users = users.Where(user => user.Lastname== lastname);
if (id.HasValue)
users = users.Where(user => user.Id == id);
return users;
}
Now, we have another search for some other data where we got one giant search value object (30+ search properties) and one method that filters it all. Applying this to the user situation, it'd look like this:
SearchUser()
{
// Get values from somewhere
var searchVO = new UserSearchVO { Firstname = firstname, Lastname = lastname, IsActive = true };
Service.SearchUser(searchVO);
}
SearchUserSomewhereElse()
{
// Get values from somewhere
var searchVO = new UserSearchVO { Lastname = lastname, Id = id };
Service.SearchUser(searchVO);
}
// some other layer
SearchUser(UserSearchVO searchVO)
{
var users = from user in allusers
select user;
if (searchVO.IsActive.HasValue)
users = users.Where(user => user.IsActive == searchVO.IsActive);
if (!String.IsNullOrEmpty(searchVO.Firstname))
users = users.Where(user => user.Firstname == searchVO.Firstname);
if (!String.IsNullOrEmpty(searchVO.Lastname))
users = users.Where(user => user.Lastname == searchVO.Lastname);
if (searchVO.Id.HasValue)
users = users.Where(user => user.Id == searchVO.Id);
if (!String.IsNullOrEmpty(searchVO.SomeFutureValue))
users = users.Where(user => user.SomeFutureValue == searchVO.SomeFutureValue);
return users;
}
I understand that the first approach is the starting approach, I don't use a search object if I don't need one with 2 params. But I feel like violating some principles of clean code with the latter approach.
Which principles of clean code do I violate using the second approach and is there another clean way of keeping it readable while not having 5000 different methods to filter 1 object?
解决方案
Assumed you don't want to change the existing User
object significantly, you could use reflection to
iterate over the properties of
UserSearchVO
check for the equally named properties of
User
implement a generic comparison of those properties for the data types required
This approach can be extended by providing custom attributes to the properties of UserSearchVO
to declare how the comparison shall be done (like mentioned in a comment by @MarcelKirsche).
This is surely overkill for 5 properties, but if you have really more than 30, it may be worth the effort.
其他提示
I find this much more readable:
// some other layer
SearchUser(UserSearchVO searchVO)
{
var users = from user in allusers
select user;
users = MatchIsActive(users, searchVO);
users = MatchFirstname(users, searchVO);
users = MatchLastname(users, searchVO);
users = MatchId(users, searchVO);
users = MatchSomeFutureValue(users, searchVO);
return users;
}
Yes it creates a bit more work behind the scenes but it's behind the scenes where it belongs. Please don't show me everything going on all at once. These functions could be methods on UserSearchVO
.
I would go for a builder pattern:
var users = SearchUserWhich
.IsActive()
.FirstNameIs('Joe')
.LastNameIs('Random')
.Fetch();
Now you can combine the filters internally the way you want until you call .Fetch
.
Basically you are wrapping SQL in a (limited) DSL because you're not using a nice SQL DSL for whatever reason (e.g. you do need that limitation, or you want joins handled automatically, etc).
UserSearchVO is just a search expression
Your UserSearchVO
object represents a search expression. So it seems reasonable that the UserSearchVO
class would know how to convert itself into an Expression<Func<User,bool>>
, which you could then provide to the Where
clause. A simple example might look like this (thanks to this answer):
class UserSearchVO
{
public bool? IsActive { get; set; }
public bool? Firstname { get; set; }
public Expression<Func<User, bool>> ToExpression()
{
var type = typeof(User);
List<Expression> expressions = new List<Expression>();
var parameter = Expression.Parameter(type, "x");
if (IsActive.HasValue)
{
var expression = Expression.Equal(Expression.MakeMemberAccess(parameter, type.GetProperty("IsActive")), Expression.Constant(IsActive.Value));
expressions.Add(expression);
}
if (Firstname.HasValue)
{
var expression = Expression.Equal(Expression.MakeMemberAccess(parameter, type.GetProperty("Firstname")), Expression.Constant(IsActive.Value));
expressions.Add(expression);
}
//Put other properties (e.g. Lastname) here
Expression final = expressions.First();
foreach (var expression in expressions.Skip(1))
{
final = Expression.And(final, expression);
}
Expression<Func<User, bool>> predicate =
(Expression<Func<User, bool>>)Expression.Lambda(final, parameter);
return predicate;
}
}
Now in your search method you can just do this:
SearchUser(UserSearchVO searchVO)
{
var users = from user in allusers
select user;
users = users.Where(searchVO.ToExpression());
return users;
}
...which seems pretty crystal clear to me, and maintains encapsulation.
For your other methods you can leverage the first one, e.g.
SearchActiveUser(string firstname, string lastname)
{
var search = new UserSearchVO
{
IsActive = true,
Firstname = firstname,
LastName = lastname
};
return SearchUser(search);
}