HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Message Service

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
messageservicestackoverflow

Problem

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
{
public MessageService(IGenericRepository messages)
: base(messages)
{
}

public IQueryable 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 FilterMessagesForUser(AuthenticatedUser user, IQueryable 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 IsNotExpired(this IQueryable query)
{
return query.Where(_ => _.ExpireDate == null || _.ExpireDate == DateTime.MinValue || _.ExpireDate < DateTime.Today);
}
}

Solution

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

  • Use braces. Save my eyes and yourself a headache later.



-
This line is getting really long. Some new lines would help readability.

messages = messages.Where(_ => _.Locations.Select(l => l.Id).Intersect(user.Locations).Any());

Code Snippets

default:
            throw new NotSupportedException();
    }
messages = messages.Where(_ => _.Locations.Select(l => l.Id).Intersect(user.Locations).Any());

Context

StackExchange Code Review Q#120523, answer score: 3

Revisions (0)

No revisions yet.