5

I have an expression that I use a few times in several LINQ queries so I separated it out into it's own method that returns the expression. The lambda portion of the function is kind of messy looking. Anyone want to take a crack at refactoring it and making it more readable and/or smaller?

    private Expression<Func<Message, bool>> UserSelector(string username, bool sent)
    {
        return x => ((sent ? x.FromUser : x.ToUser).Username.ToLower() == username.ToLower()) && (sent ? !x.SenderDeleted : !x.RecipientDeleted);
    }

A quick English description of what it's doing is it is checking the boolean sent and checking either the Message.FromUser or the Message.ToUser based on that boolean.

If the user is looking at his/her outbox, sent is true and it will see if x.FromUser.Username == username and x.SenderDeleted == false.

If the user is looking at his/her inbox then it does the same logic, but sent is false and it checks x.ToUser and x.RecipientDeleted instead.

Maybe this is the easiest way but I'm open to some refactoring.

ANSWER EDIT

I really liked Davy8's answer but I decided to take a step further and do two expressions instead of one expression with a nested function. Now I have the following methods:

    /// <summary>
    /// Expression to determine if a message belongs to a user.
    /// </summary>
    /// <param name="username">The name of the user.</param>
    /// <param name="sent">True if retrieving sent messages.</param>
    /// <returns>An expression to be used in a LINQ query.</returns>
    private Expression<Func<Message, bool>> MessageBelongsToUser(string username, bool sent)
    {
        return x => (sent ? x.FromUser : x.ToUser).Username.Equals(username, StringComparison.OrdinalIgnoreCase);
    }

    /// <summary>
    /// Expression to determine if a message has been deleted by the user.
    /// </summary>
    /// <param name="username">The name of the user.</param>
    /// <param name="sent">True if retrieving sent messages.</param>
    /// <returns>An expression to be used in a LINQ query.</returns>
    private Expression<Func<Message, bool>> UserDidNotDeleteMessage(string username, bool sent)
    {
        return x => sent ? !x.SenderDeleted : !x.RecipientDeleted;
    }

So now my queries look like this:

    /// <summary>
    /// Retrieves a list of messages from the data context for a user.
    /// </summary>
    /// <param name="username">The name of the user.</param>
    /// <param name="page">The page number.</param>
    /// <param name="itemsPerPage">The number of items to display per page.</param>
    /// <param name="sent">True if retrieving sent messages.</param>
    /// <returns>An enumerable list of messages.</returns>
    public IEnumerable<Message> GetMessagesBy_Username(string username, int page, int itemsPerPage, bool sent)
    {
        var query = _dataContext.Messages
            .Where(MessageBelongsToUser(username, sent))
            .Where(UserDidNotDeleteMessage(username, sent))
            .OrderByDescending(x => x.SentDate)
            .Skip(itemsPerPage * (page - 1))
            .Take(itemsPerPage);
        return query;
    }

    /// <summary>
    /// Retrieves the total number of messages for the user.
    /// </summary>
    /// <param name="username">The name of the user.</param>
    /// <param name="sent">True if retrieving the number of messages sent.</param>
    /// <returns>The total number of messages.</returns>
    public int GetMessageCountBy_Username(string username, bool sent)
    {
        var query = _dataContext.Messages
            .Where(MessageBelongsToUser(username, sent))
            .Where(UserDidNotDeleteMessage(username, sent))
            .Count();
        return query;
    }

I would say those are some very English readable queries, thanks guys!

Reference: http://www.codetunnel.com/blog/post/64/how-to-simplify-complex-linq-expressions

CatDadCode
  • 58,507
  • 61
  • 212
  • 318

3 Answers3

3

Split it out into a separate function:

    private Expression<Func<Message, bool>> UserSelector(string username, bool sent)
    {
        return message=> InnerFunc(message, username, sent);
    }

    private static bool InnerFunc(Message message, string username, bool sent)
    {
        if(sent)
        {
            return string.Equals(message.FromUser.Username, username, StringComparison.InvariantCultureIgnoreCase) && !message.SenderDeleted;
        }
        return string.Equals(message.ToUser.Username, username, StringComparison.InvariantCultureIgnoreCase) && !message.RecipientDeleted;
    }

Or alternatively it can be inlined to maintain closure usage:

    private Expression<Func<Message, bool>> UserSelector(string username, bool sent)
    {
        Func<Message, bool> innerFunc = message =>
        {
            if (sent)
            {
                return string.Equals(message.FromUser.Username, username, StringComparison.InvariantCultureIgnoreCase) &&
                        !message.SenderDeleted;
            }
            return string.Equals(message.ToUser.Username, username, StringComparison.InvariantCultureIgnoreCase) &&
                    !message.RecipientDeleted;
        };
        return message => innerFunc(message);
    }

(Edited to use string.Equals with StringComparison.InvariantCultureIgnoreCase for odd edge cases with different culture settings.)

Davy8
  • 30,868
  • 25
  • 115
  • 173
  • I like the idea of two functions and doing two `.Where` chains on the collection, one for each expression. This is very readable. – CatDadCode May 26 '11 at 17:47
  • 1
    Take a look at my edit for my implementation of your answer :) – CatDadCode May 26 '11 at 18:03
  • @Davy8 I deleted my answer but I think the [link](http://stackoverflow.com/questions/3678792/are-string-equals-and-operator-really-same) to Jeffrey L Whitledge's answer to [are-string-equals-and-operator-really-same](http://stackoverflow.com/questions/3678792/are-string-equals-and-operator-really-same) might put the InvariantCultureIgnoreCase bit in context – Conrad Frix May 26 '11 at 19:08
  • Believe it or not I actually had to remove the string.Equals from my expression. It didn't like it. I had to go back to `.ToLower()`. The app would compile, but then at runtime it would say "invalid number of arguments supplied for method string.Equals". I triple checked the arguments, even debugged it and they were getting passed. Just didn't like it in an expression. If I did it up in the main method it worked fine, just not in the expression. – CatDadCode May 26 '11 at 19:11
  • Okay, I got string.Equals to work, but only by calling its instance equivalent. Instead of `string.Equals(str, str, ignoreCase)` I call `str.Equals(str, ignoreCase)` and that seems to work okay in an expression. Seems that calling the static string class was the issue. – CatDadCode May 26 '11 at 19:56
0

Do you have Resharper? A lot of times I'll write something like this as a foreach loop and see if Resharper can refactor it. I think readability, though, is most important, although you should evaluate the peformance of any solution you come up with.

Stealth Rabbi
  • 10,156
  • 22
  • 100
  • 176
0

As long as you write the description you wrote for this question as a comment, it should help anyone trying to read / understand it correct?

Viv
  • 2,515
  • 2
  • 22
  • 26
  • Right, I'm just seeing what you guys can come up with that's all. I'm often surprised by how much easier people manage to do things than I thought possible. – CatDadCode May 26 '11 at 17:26