snippetcsharpMinor
How can the below code can be refactored with design pattern?
Viewed 0 times
canthewithdesignrefactoredbelowhowcodepattern
Problem
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:
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("{
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.
```
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("{
Solution
I would look at replacing the BLL code with the following. With the main focus being replacing the
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
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.Code Snippets
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;
}
}
}Context
StackExchange Code Review Q#7306, answer score: 5
Revisions (0)
No revisions yet.