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

• Pimgd: Welcome to Code Review, would you mind describing in more detail what parts of your code don't feel clean?
• Imran Rizvi: 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.
• Bryan Vam: 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?
• 404: I would suggest looking at FilterMessagesForUser from the perspective of "Tell, Don't Ask" - you are asking User a lot about its state.

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

• Dan Lyons: There's actually a specialized ArgumentException type he could use here: InvalidEnumArgumentException
• RubberDuck: Even better yet @DanLyons!