4
\$\begingroup\$

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
}
\$\endgroup\$
2
  • \$\begingroup\$ It's good to be aware what is slow, but unless you actually have a good reason to worry about the performance of this code, don't base your decisions on it. \$\endgroup\$
    – svick
    Commented Dec 22, 2013 at 0:57
  • \$\begingroup\$ Though using reflection is a code smell and you are right to ask about ways of avoiding it. \$\endgroup\$
    – svick
    Commented Dec 22, 2013 at 1:03

2 Answers 2

5
\$\begingroup\$

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))
\$\endgroup\$
3
  • \$\begingroup\$ class User {} Did you mean to make User inherit from Settings, just like in the original design? Though I'm not sure that's actually a good idea. \$\endgroup\$
    – svick
    Commented Dec 22, 2013 at 1:01
  • \$\begingroup\$ @svick Yes. Fixed it. \$\endgroup\$ Commented Dec 22, 2013 at 7:47
  • \$\begingroup\$ @svick There are other points as you point out. User is not a Settings, User has a Settings. CreateNotification should be just a factory method. Number of NotificationX suggest some sort of event setup. \$\endgroup\$ Commented Dec 22, 2013 at 9:01
2
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ Why not just pull the value from the Dictionary? If it's an invalid value, you'll have your exception for free! :) \$\endgroup\$ Commented Dec 20, 2013 at 16:57
  • 2
    \$\begingroup\$ Because then you get some stupid, unintelligible exception message. This way you can actually throw somethign useful. \$\endgroup\$ Commented Dec 20, 2013 at 17:51
  • \$\begingroup\$ The KeyNotFoundException would (should?) never be thrown in production code, so a custom exception makes no sense to me. "The given key was not present in the dictionary" says exactly what's going on, and the stack trace will point to Dictionary.GetValue in the very method you're doing that, which is all you need to debug the issue. I don't see the added value of a custom exception message in this case, it's overkill. \$\endgroup\$ Commented Dec 20, 2013 at 18:44
  • \$\begingroup\$ enum's are trivially serializable, delegates not. How would you store notification setting for each user in db? \$\endgroup\$ Commented Dec 21, 2013 at 18:43
  • \$\begingroup\$ The enums are still stored in the DB. The delegates are replacing te handling code, not the actual values. \$\endgroup\$ Commented Dec 21, 2013 at 20:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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