5
\$\begingroup\$

Below is the code to a Message Service. Does anyone have any ideas on how to improve the methods. It doesn't feel clean to me.

public enum MessagesType
{
    Standard,
    LocationTargeted,
    Scheduled,
    Expired
}

public class MessageService : GenericService<PushMessage>
{
    public MessageService(IGenericRepository<PushMessage> messages)
        : base(messages)
    {
    }

    public IQueryable<PushMessage> Get(AuthenticatedUser user, MessagesType type)
    {
        var messages = this.Get();
        switch (type)
        {
            case MessagesType.Standard:
                messages = messages.Where(_ => string.IsNullOrEmpty(_.LocationLatitude) && _.ReleaseDate == null).IsNotExpired();
                break;
            case MessagesType.LocationTargeted:
                messages = messages.Where(_ => !string.IsNullOrEmpty(_.LocationLatitude)).IsNotExpired();
                break;
            case MessagesType.Scheduled:
                messages = messages.Where(_ => _.ReleaseDate > DateTime.MinValue);
                break;
            case MessagesType.Expired:
                messages = messages.Where(_ => _.ReleaseDate > DateTime.MinValue && _.ExpireDate < DateTime.Today);
                break;
            default:
                throw new NotSupportedException();
        }

        return this.FilterMessagesForUser(user, messages);
    }

    private IQueryable<PushMessage> FilterMessagesForUser(AuthenticatedUser user, IQueryable<PushMessage> messages)
    {
        if (user.Role == Role.Administrator)
            return messages;

        if (user.Apps.Length > 0)
        {
            if (user.Locations.Length > 0)
                messages = messages.Where(_ => _.Locations.Select(l => l.Id).Intersect(user.Locations).Any());
            else
                messages = messages.Where(_ => user.Apps.Contains(_.AppId));
        }
        else
        {
            messages = messages.Where(_ => _.App.ClientId == user.ClientId);
        }

        return messages;
    }
}

public static class MessageExtensions
{
    public static IQueryable<PushMessage> IsNotExpired(this IQueryable<PushMessage> query)
    {
        return query.Where(_ => _.ExpireDate == null || _.ExpireDate == DateTime.MinValue || _.ExpireDate < DateTime.Today);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review, would you mind describing in more detail what parts of your code don't feel clean? \$\endgroup\$ – Pimgd Feb 19 '16 at 12:50
  • \$\begingroup\$ What is not clean, first look it looks clean, if your code is complex to understand than you should write comments at each complex statement or at function level to make it easier to understand. \$\endgroup\$ – Imran Rizvi Feb 19 '16 at 13:14
  • \$\begingroup\$ It's specifically the FilterMessagesForUser method. It feels a bit too busy. I am not sure if user permissions should be handled in a service class (business layer)? Should it be handled somewhere else? \$\endgroup\$ – Bryan Vam Feb 19 '16 at 13:26
  • \$\begingroup\$ I would suggest looking at FilterMessagesForUser from the perspective of "Tell, Don't Ask" - you are asking User a lot about its state. \$\endgroup\$ – 404 Feb 21 '16 at 22:19
3
\$\begingroup\$

Honestly, it looks fine to my eyes. There are a few things that jump out at me, but all fairly minor.

This is the biggest red flag to me.

        default:
            throw new NotSupportedException();
    }

It's the wrong exception type. The msdn doc says:

The exception that is thrown when an invoked method is not supported, or when there is an attempt to read, seek, or write to a stream that does not support the invoked functionality.

Which seems close at a glance, but really doesn't hit the nail on the head. An ArgumentException would be much more appropriate here. Even better, use an ArgumentOutOfRange exception to perfectly describe what went wrong.


Personal Preference Nitpicks

  1. Use braces. Save my eyes and yourself a headache later.
  2. This line is getting really long. Some new lines would help readability.

    messages = messages.Where(_ => _.Locations.Select(l => l.Id).Intersect(user.Locations).Any());
    
\$\endgroup\$
  • 1
    \$\begingroup\$ There's actually a specialized ArgumentException type he could use here: InvalidEnumArgumentException \$\endgroup\$ – Dan Lyons Feb 25 '16 at 18:39
  • \$\begingroup\$ Even better yet @DanLyons! \$\endgroup\$ – RubberDuck Feb 25 '16 at 19:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.