Are redundancy and reflection my only options here?

Posted on

Problem

I have a function – TriggerNotifications – that accepts a NotificationType. The Type determines which settings I should pull from the User.

The code looks like this:

public void TriggerNotification(DbContext db, User user, NotificationType type, string content) {
            switch (type) {
                case NotificationType.Followed:
                    CreateNotification(db, user, user.Followed, content);
                    return;
                case NotificationType.Unfollowed:
                    CreateNotification(db, user, user.Unfollowed, content);
                    return;
                case NotificationType.Messaged:
                    CreateNotification(db, user, user.Messaged, content);
                    return;
                default: 
                    throw new InvalidDataContractException("Invalid Notification Type");
            }
        }

So the only purpose of this method is to determine which settings should be used. Every User has a property matching each of the enum NotificationTypes.

I think this is a good use case for reflection, but want to ask if there’s any other way to reduce redundancy here. I don’t like reflection because it’s not as straightforward to debug, and it’s slow (right?).

UPDATE

CreateNotification:

private void CreateNotification(DbContext db, User user, NotificationSetting setting, string content) {
        // send email, notification, or both
        if (setting.Equals(NotificationSetting.None))
            return;
        var notification = new Notification() {
            UserId = user.userId
            Description = content,
            Email = email,
        };

        if (setting.Equals(NotificationSetting.Email) || setting.Equals(NotificationSetting.Both)) {
            // tell worker role to pick it up
            notification.PendingEmail = true;
        }

        // signalR stuff here

        db.Add(notification);
    }

User:

public class User : Settings {
      // other stuff
}

Settings:

public abstract class Settings {
    // notifications
    public NotificationSetting Followed { get; set; }
    public NotificationSetting Unfollowed { get; set; }
    public NotificationSetting Messaged { get; set; }
}

NotificationSetting:

// values stored in the db for each setting property - messaged, unfollowed, etc
public enum NotificationSetting {
    Navbar,
    Email,
    Both,
    None
}

NotificationType:

// values used in logic only to tell the notification builder which setting to pull
public enum NotificationType {
    Followed,
    Unfollowed,
    Messaged
}

Solution

Some of the redundancy is accidental, and some essential to your design (not essential to problem though, if it were essential to problem reflection wouldn’t help).

Accidental redundancy can be removed thus:

private static NotificationSetting GetNotificationSetting(User user, NotificationType type)
{
    switch (type) {
    case NotificationType.Followed: return user.Followed;
    case NotificationType.Unfollowed: return user.Unfollowed;
    case NotificationType.Messaged: return user.Messaged;
    default: 
        throw new InvalidEnumArgumentException("Invalid Notification Type");
    }
}

public void TriggerNotification(object db, User user, NotificationType type, string content) {
    CreateNotification(db, user, GetNotificationSetting(user,type), content);
}

Note:

  • I changed the exception as InvalidDataContractException seems to be serialization related.

  • GetNotificationSetting should be moved to User (or made an extension method of that class).

Now that this is out of the way,

What can you change in the design to remove redundancy?

Your user entity is not normalized. If you really were doing code-first design, User would look like this instead:

class User: Settings
{
}

public abstract class Settings {
    public IDictionary<NotificationType, NotificationSetting> NotificationSettings { get; set; }
}

Then TriggerNotification becomes just:

public void TriggerNotification(object db, User user, NotificationType type, string content) {
    CreateNotification(db, user, user.NotificationSettings[type], content);
}

Another redundancy is in NotificationSetting. It clearly is a bitmask.

[Flags]
public enum NotificationSetting : uint
{
  Navbar = 1,
  Email = 2,
// can add more like so
  Telegram = 4,
  PersonalVisit = 8,
}

Then

if (setting.Equals(NotificationSetting.Email) || setting.Equals(NotificationSetting.Both))

becomes

if (setting.HasFlag(NotificationSetting.Email))

The redundancy is hard to remove and constructions like these make it hard to extend the options later. It would require updates to both data contracts and all classes implementing them.

You could use a delegate instead of a switch/case to improve extensibility, it would not completely remove the redundancy:

var actions = Dictionary<NotificationType, Func<DbContext, User, content>>()
{
    { 
        NotificationType.Followed, (context, user, content) => CreateNotification(context, user.Followed, content),
        NotificationType.Messaged, (context, user, content) => CreateNotification(context, user.Messaged, content),
        NotificationType.Unfollowed, (context, user, content) => CreateNotification(context, user.Unfollowed, content)
     };
 }

That way you can invoke it like this:

 Action<DbContext, User, content>> action = null;
 if (actions.TryGetValue(notification, out action)
 {
     action.Invoke(context, user, content);
 }
 else
 {
    // throw exception?
 }

You can also change your baseclass to be more extensible:

 public abstract class Settings
 {

     void SetNotificationAction(NotificationType nt, Func action)
     {
         // you could also replace the action if you so desire
         actions.Add(nt, action);
     }

     void ClearNotificationAction(NotificationType nt)
     {
         // you could also replace the action if you so desire
         actions.Remove(nt);
     }

     virtual void SetDefaultActions()
     {
         SetNotificationAction(NotificationSetting.Followed, () => return );
         ...
         ...
     }
 }

That does away with the enum passing and you can fetch the action using the method above. Each class inheriting can override the SetDefaultActions or manipulate the actions in the actions dictionary.

Leave a Reply

Your email address will not be published. Required fields are marked *