Message Service

Question

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

Answer 1

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