# How can the below code can be refactored with design pattern?

In one of our project with 3 tier architecture (tightly coupled), there's a bad code smell and it doesn't follow the 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
{
{

{
SignUPFBPost = (Convert.ToInt16(dtblNotifList.Rows[0]["FBWallPost"]) == 1) ? true : false;
SignUPEmails = (Convert.ToInt16(dtblNotifList.Rows[0]["Emails"]) == 1) ? true : false;
SignUPFCActivity = (Convert.ToInt16(dtblNotifList.Rows[0]["FCActivity"]) == 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.

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);
{
Response.Write(Javascript.ScriptStartTag + "PostOnFBWall(\"" + strMessage + "\",\"" + Config.FacebookAppUrl + "\",\"" + Config.WebSiteUrl + "images/logo.png\",\"Favor in Progress!\",\"\",\"Message\");" + Javascript.ScriptEndTag);
}
{
}
{
}
break;
//Other Cases
}
}


How can we refector the code with or without a 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.

-

## migrated from programmers.stackexchange.comDec 30 '11 at 12:19

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

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

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 String NotificationMailMessage { get; };
public String NotificationPushMessage { get; };
}

//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
{
//you could replace this with a general message and some parameters if applicable
{
}

//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
}

{
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";


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.

-
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

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
{
}

// sample implementation of the interface
{
{
// do whatever you need to return information from your data repository
throw new System.NotImplementedException();
}
}

{
// 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; }

{ }

// using an interface allows mocking of the 'NotificationSettingsBAL' object
{
// prefer suffixes over prefixes for naming conventions

SignUPFBPost = TryParseBool("FBWallPost");
SignUPEmails = TryParseBool("Emails");
SignUPFCActivity = TryParseBool("FCActivity");
}

// 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 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.

-
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

• 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.