Grouping of methods

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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).

Answer 4

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);
}