Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In one of our project with 3 tier architecture (tightly coupled), there's bad code smell. It doesn't follow DRY principle. I want to refactor it with design possible design pattern. I don't want to write this project from scratch.

Here's one of the class in BLL :

namespace BAL
{
   public class NotificationSettings
   {
    public readonly bool SignUPFBPost;
    public readonly bool SignUPEmails;
    public readonly bool SignUPPushNotification;
    public readonly bool SignUPFCActivity;
    public readonly bool SignUPBubbleNotification;
    public readonly bool SignUPGlobeNotification;

    public NotificationSettings()
    {
        NotificationSettingsBAL objNotificationSettings = new NotificationSettingsBAL();
        DataTable dtblNotifList = objNotificationSettings.GetNotificationList();
        SignUPFBPost = (Convert.ToInt16(dtblNotifList.Rows[0]["FBWallPost"]) == 1) ? true : false;
        SignUPEmails = (Convert.ToInt16(dtblNotifList.Rows[0]["Emails"]) == 1) ? true : false;
        SignUPPushNotification = (Convert.ToInt16(dtblNotifList.Rows[0]["PushNotification"]) == 1) ? true : false;
        SignUPFCActivity = (Convert.ToInt16(dtblNotifList.Rows[0]["FCActivity"]) == 1) ? true : false;
        SignUPBubbleNotification = (Convert.ToInt16(dtblNotifList.Rows[0]["BubbleNotification"]) == 1) ? true : false;
        SignUPGlobeNotification = (Convert.ToInt16(dtblNotifList.Rows[0]["GlobNotification"]) == 1) ? true : false;
    }
   }
}   

We're getting various user related settings db from database and checking at different places as per the need and displaying messages or sending push notification. Check Sample code below.

if (objSqlResult.IsSuccess)
{
FavorBAL.Favor objFavorStatus = (FavorBAL.Favor)objFavorBAL.Status;

switch (objFavorStatus)
{
    case FavorBAL.Favor.Initiated:
        break;
    case FavorBAL.Favor.Accepted:
        strTitle = "Accept";
        strMessage = MsgSuccess.GetMsg(Successes.FBPostFavorAccept).Replace("{%RecMemberName%}", objMemberBAL.FirstName + " " + objMemberBAL.LastName).Replace("{%ReqMemberName%}", Convert.ToString(dtblFavor.Rows[0]["ReqMemberName"]));
        strEmailMessage = MsgSuccess.GetMsg(Successes.FBPostFavorAccept).Replace("{%RecMemberName%}", objMemberBAL.FirstName + " " + objMemberBAL.LastName).Replace("{%ReqMemberName%}", "you");
        Common.DisplayMessageFavor(divMsg, MsgSuccess.GetMsg(Successes.FriendFavorAccept), Common.ErrorMsgType.Success);
        if (new NotificationSettings().FavorAcceptFBPost)
        {
            Response.Write(Javascript.ScriptStartTag + "PostOnFBWall(\"" + strMessage + "\",\"" + Config.FacebookAppUrl + "\",\"" + Config.WebSiteUrl + "images/logo.png\",\"Favor in Progress!\",\"\",\"Message\");" + Javascript.ScriptEndTag);
        }
        if (new NotificationSettings().FavorAcceptEmails)
        {
            IsNotificationMailAllowed = true;
        }
        if (new NotificationSettings().FavorAcceptPushNotification)
        {
            objPushNotification.Push(strMessage, strUDID);
        }
        break;
        //Other Cases
}
}

How we could refector the code with or without design pattern? There's replication of second code listing or similar to it. I am reading one of the article on interface to make it loosely coupled so that system can adapt changes easily.

share|improve this question
1  
repeated stuff like (Convert.ToInt16(dtblNotifList.Rows[0]["GlobNotification"]) == 1) ? true : false looks like a candidate for my favorite Extract method pattern –  gnat Dec 30 '11 at 8:40
    
@gnat For brevity I've not posted complete code. Constructor is having total 15 boolean variables (e.g signup) which are assigned values based on table index. How could we extract method in that case? –  Nilesh Thakkar Dec 30 '11 at 11:05
    
for example of extracted method in your case, see TryParseBool in an answer from Kane - "main focus being replacing the Convert.ToInt16 repetition with a method" –  gnat Dec 30 '11 at 12:03
    
Even if I replace Convert.ToInt16 with TryParseBool, I am repeatedly calling TryParseBool for converting datatable value to boolean value, so what difference does it make? –  Nilesh Thakkar Jan 2 '12 at 4:53
    
As of now there are only six notification settings for each kind of activity (total 15 means total 90 fields) as displayed in code. If at all tomorrow one more setting will be added user wise, I might have to change the code. I think that has to be changed using Design Pattern and Reflection, can anybody throw some light? –  Nilesh Thakkar Jan 2 '12 at 13:36
show 1 more comment

migrated from programmers.stackexchange.com Dec 30 '11 at 12:19

This question came from our site for professional programmers interested in conceptual questions about software development.

3 Answers

up vote 1 down vote accepted

I'm not really sure what you're trying to do but I think the purpose of your code is to check what kind of notifications are enabled and perform them accordingly. Assuming that the notification settings are on a per-user basis (as a setting for example) and not something that can be enabled/disabled application wide I would make the following changes:

Create a general INotification-interface and subclass from that interface

//depending on how different the messages are per type of notification
//you might only need one message
public interface INotification
{
    public String NotificationMailMessage { get; };
    public String NotificationPushMessage { get; };
    //add more types here
}

//you might only need one concrete implementation, dependings on how complex the logic is
//using the INotification-interface gives you the flexibility to create specific
//notification classes for more complex notifications
public class SimpleNotification: INotification
{
    //you could replace this with a general message and some parameters if applicable
    public SimpleNotification(String mailMessage, String pushMessage)
    {
        NotificationMailMessage = mailMessage;
        NotificationPushMessage = pushMessage;
    }

    //implement properties here
}

Something you could do is:

public class FavorAcceptedNotification : INotification
{
    private String _firstName;
    private String _lastName;
    private String _reqMemberName;

    public FavorAcceptedNotification(String firstName, String lastName, String reqMembername)
    {
        _firstName = firstName;
        //and so on
    }

    public String NotificationMailMessage
    {
        get
        {
            return MsgSuccess.GetMsg(Successes.FBPostFavorAccept).Replace("{%RecMemberName%}", _firstName + " " + _lastName).Replace("{%ReqMemberName%}", "you");
        }
    }
}

Which means you simplify the switch to:

case FavorBAL.Favor.Accepted:
    strTitle = "Accept";
    var notification = new FavorAcceptedNotification(objMemberBAL.FirstName,  objMemberBAL.LastName, Convert.ToString(dtblFavor.Rows[0]["ReqMemberName"]));
    var notificationSettings = new NotificationSettings();
    notificationSettings.Push(notification);

Your NotificationSettings class (might need to rename that one since it now does more then wrap settings) can deal with determining what notifications are enabled and what needs to be sent. Use Kane's suggestion to refactor with TryParseBool.

It all depends on what you're trying to achieve really.

share|improve this answer
    
Thanks JDT for replying. You're right that Notification Settings are stored user wise and not application wise. I think I need to use Strategy pattern for switch..case. Notificatin Setting class just get the settings user wise from DB. I think I need to create one more class which will pull System message from XML file and will be displayed based on action performed and user setting for Notification. But bit confused from Where should I start? –  Nilesh Thakkar Jan 2 '12 at 5:36
    
If you mean the class that will actually do the work - just create a class with a methode that takes INotification objects and acts on them based on the NotificationSettings. –  JDT Jan 3 '12 at 5:28
add comment

I would look at replacing the BLL code with the following. With the main focus being replacing the Convert.ToInt16 repetition with a method. The other area of concern is the inability to mock your BAL code. To overcome this I have written a quick and dirty interface which allows better unit testing.

namespace BAL
{
    // interfaces allow the class ot be easily mocked
    internal interface ICustomDataColumnTryParse
    {
        bool TryParseBool(string columnName);
    }

    // interfaces allow the class ot be easily mocked
    internal interface INotificationSettingsDomain
    {
        DataTable GetNotificationList();
    }

    // sample implementation of the interface
    internal class NotificationSettingsBAL : INotificationSettingsDomain
    {
        public DataTable GetNotificationList()
        {
            // do whatever you need to return information from your data repository
            throw new System.NotImplementedException();
        }
    }

    internal class NotificationSettings : ICustomDataColumnTryParse
    {
        // properties are more widely accepted for consumers of this class over
        // traditional fields / variables, but it really depends on 'what' you want
        // to use these values for?
        internal bool SignUPFBPost { get; private set; }
        internal bool SignUPEmails { get; private set; }
        internal bool SignUPPushNotification { get; private set; }
        internal bool SignUPFCActivity { get; private set; }
        internal bool SignUPBubbleNotification { get; private set; }
        internal bool SignUPGlobeNotification { get; private set; }

        private DataTable m_notificationListDataTable;

        internal NotificationSettings()
            : this(new NotificationSettingsBAL())
        { }

        // using an interface allows mocking of the 'NotificationSettingsBAL' object
        public NotificationSettings(INotificationSettingsDomain notificationSettingsDomain)
        {
            // prefer suffixes over prefixes for naming conventions
            m_notificationListDataTable = notificationSettingsDomain.GetNotificationList();

            SignUPFBPost = TryParseBool("FBWallPost");
            SignUPEmails = TryParseBool("Emails");
            SignUPPushNotification = TryParseBool("PushNotification");
            SignUPFCActivity = TryParseBool("FCActivity");
            SignUPBubbleNotification = TryParseBool("BubbleNotification");
            SignUPGlobeNotification = TryParseBool("GlobNotification");
        }

        // this can be a method on this class, extension method (my preference) or a static method
        // i have opted for an internal class method coded against an interface so the data table 
        // isn't passed around. 
        public bool TryParseBool(string columnName)
        {
            // this can allow for additional logic if we wanted to get a different row index
            var row = m_notificationListDataTable.Rows[0];
            var value = row[columnName];

            if (value == null)
            {
                return false;
            }

            int valueAsInt = 0;
            int.TryParse(value.ToString(), out valueAsInt);
            return valueAsInt == 1;
        }
    }
}

As for replacing the switch statement you might be over-using an architectural pattern. I would only choose something like a strategy pattern if the business logic is likely to change or that additional switch statements will be added / removed with a high frequency in the future.

share|improve this answer
    
You're right Kane, that I've to use Strategy Pattern to replace my long swith..case. But I am not sure how should I go about replacing it? –  Nilesh Thakkar Jan 2 '12 at 5:45
    
I might be able to instantiate the NotificationSettings but won't be able to access properties as they're set Internal. –  Nilesh Thakkar Jan 2 '12 at 6:53
    
Thanks for answering, I appreciate :) –  Nilesh Thakkar Jan 3 '12 at 9:24
add comment

I would start with those steps:

  • Create an enum for your flags 'FBWallPost', 'Emails', and so on.
  • Put yor "signs" into an boolean array indexed by values of that enum (instead of individual booleans), then you can easily create a loop around that.

After that replacement I would check how much of the second part of your code can be refactored to general methods. Then I would look at the remaining parts and how much code duplication is still in there. Eventually I would try to apply strategy pattern, having classes FBWallPostStrategy, EmailsStrategy etc. with a common base class. Those classes may later replace the enum I have introduced first (you won't need it then anymore), keep only the individual code parts that are special for the particular case. The common code goes either to the common base class or a separate class applying "template method" pattern.

share|improve this answer
    
NotificationSetting class fetches total 5 boolean values for each of the application activities like - FacebookWallPost - Emails - PushNotification - DisplayActivityOnWebSiteWall - FacebookGlobNotification. Whenever any activity performed on website, settings for this variable checked for given user and particular action gets performed. –  Nilesh Thakkar Dec 30 '11 at 11:17
    
Thanks for the answering –  Nilesh Thakkar Jan 3 '12 at 9:23
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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