Neo Neo - 28 days ago 5
C# Question

How do I best refactor this C# code?

I want to refactor the following code, how do I inject notification handler? And minimum original code changes and best refactoring where necessary.

public class TestEventHandlers
{
public TestEventHandlers() { }

public void OpenMarket(Page page)
{
var Id = page.Request["MarketId"];

var repository = new EntityRepository();
IEntity market = repository.GetById(Id);

if (market.State != "Open")
{
throw new Exception("The market is not open!");
}
else
{
market.Open();

repository.SaveChangesTo(market);

var smtpClient = new SmtpClient();

var message = new MailMessage();
message.Subject = "market open";
message.Body = market.ToString() + " was open.";
message.To.Add(new MailAddress("market@mail.com"));

smtpClient.Send(message);
}
}

public void CloseMarket(Page page)
{
var Id = page.Request["MarketId"];
var repository = new EntityRepository();
IEntity market = repository.GetById(Id);

if (market.State == "Close")
{
throw new Exception("The market is already close!");
}
else
{
market.Close();

repository.SaveChangesTo(market);

var smtpClient = new SmtpClient();

var message = new MailMessage();
message.Subject = "market closed";
message.Body = market.ToString() + " has been closed.";
message.To.Add(new MailAddress("market@mail.com"));

smtpClient.Send(message);
}
}
}


I have already refactored it like below -

public class TestEventHandlers
{
public TestEventHandlers() { }

public void OpenMarket(Page page)
{
var Id = page.Request["MarketId"];
if (id!=null)
{
var repository = new EntityRepository();
IEntity market = repository.GetById(Id);

if (market.State != "Open")
{
throw new Exception("The market is not open!");
}
else
{
market.Open();

repository.SaveChangesTo(market);

SendEmailNotification("market open", market.ToString() + " was open.", "market@mail.com");
}
}
else
{
throw new Exception("Id can not be null");
}
}

private static void SendEmailNotification(string subject,string body,string emailAddress)
{
var smtpClient = new SmtpClient();

var message = new MailMessage();

message.Subject = subject;
message.Body = body;
message.To.Add(new MailAddress(emailAddress));

smtpClient.Send(message);
}

public void CloseMarket(Page page)
{
var Id = page.Request["MarketId"];
if(id!=null)
{
var repository = new EntityRepository();
IEntity market = repository.GetById(Id);

if (market.State == "Close")
{
throw new Exception("The market is already close!");
}
else
{
market.Close();

repository.SaveChangesTo(market);

SendEmailNotification("market closed", market.ToString() + " has been closed.", "market@mail.com");
}
}
else
{
throw new Exception("Id can not be null");
}
}
}

Answer

I'd go with something like this:

public interface INotifier
{
    void SendEmailNotification(string subject, string body, string emailAddress);
}

public class Notifier : INotifier
{
    public void SendEmailNotification(string subject,string body,string emailAddress)
    {
        var smtpClient = new SmtpClient();

        var message = new MailMessage();

        message.Subject = subject;
        message.Body = body;
        message.To.Add(new MailAddress(emailAddress));

        smtpClient.Send(message);
    }
}

public class TestEventHandlers
{       
    public INotifier Notifier { get; set; }

    public TestEventHandlers()
    {           
        Notifier = new Notifier();
    }

    public void OpenMarket(Page page)
    {
        var Id = page.Request["MarketId"];
        if (id!=null)
        { 
            var repository = new EntityRepository();
            IEntity market = repository.GetById(Id);

            if (market.State != "Open")
            {
                throw new Exception("The market is not open!");
            }
            else
            {
                market.Open();

                repository.SaveChangesTo(market);

                Notifier.SendEmailNotification("market open", market.ToString() + " was open.", "market@mail.com");
            }
        }
        else
        {
            throw new Exception("entityId can not be null");
        }           
    }

    public void CloseMarket(Page page)
    {
        var Id = page.Request["MarketId"];
        if(id!=null)
        {
            var repository = new EntityRepository();
            IEntity market = repository.GetById(Id);

            if (market.State == "Close")
            {
                throw new Exception("The market is already close!");
            }
            else
            {
                market.Close();
                repository.SaveChangesTo(market);
                Notifier.SendEmailNotification("market closed", market.ToString() + " has been closed.", "market@mail.com");
            }
        }
        else
        {
            throw new Exception("entityId can not be null");
        }
    }       
}

In this your INotifier is defaulted to your usual implementation by the constructor, but able to be overridden when required using the accessors. If you'd prefer to always inject in your dependencies even when not testing, you can just add arguments to the constructor.

Hope this helps.

Comments