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 toUser
(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.