0
\$\begingroup\$

In my application I have an Interface IEmailNotification that represents an Email and the concrete implementations have nothing to do with MailMessage class so to be able to send it through SmtpClient I used an adapter to let the communication between SmtpClient and IEmailNotification possible and the job of this adapter is simply delegate the Send call to SmtpClient but after converting from EmailNotification to MailMessage using an abstract Factory which in fact only copying the values from one object to the new created MailMessage object.

The first question is: my factory method takes a parameter of type IEmailNotification, does that violates the main Job of a Factory which is only creating objects?

public interface INotification
    {
        string To { get; }
        string Body { get; }
    }

public interface IEmailNotification : INotification
    {
        string From { get; }
        string Subject { get; }
        bool IsBodyHtml { get; }
        string CC { get;  }
        string BCC { get;  }
        string ReplyToList { get;  }
        List<string> AttachmentsPaths { get; }
    }

public interface IEmailNotificationService : IService
    {
        void Send(IEmailNotification notification);
    }

public class EmailNotificationServiceAdapter : IEmailNotificationService
    {
        private ISmtpClient _client;
        private MailMessageFactory _mailMessageFactory;

        public EmailNotificationServiceAdapter(ISmtpClient client, MailMessageFactory mailMessageFactory)
        {
            _client = client;
            _mailMessageFactory = mailMessageFactory;
        }

        public void Dispose()
        {
            _client.Dispose();
        }

        public void Send(IEmailNotification notification)
        {
            **using (var mailMessage = _mailMessageFactory.CreateMailMessage(notification))**
            {
                _client.Send(mailMessage);
            }
        }
    }

public class EmailMailMessageFactory : MailMessageFactory
{
    public EmailMailMessageFactory(string backupBccEmail)
        : base(backupBccEmail)
    {
    }

    public override MailMessage CreateMailMessage(IEmailNotification emailNotification)
    {
        using (var mailMessage = new MailMessage())
        {
            mailMessage.From = new MailAddress(emailNotification.From);
            mailMessage.To.Add(emailNotification.To);
            mailMessage.Subject = emailNotification.Subject;
            mailMessage.Body = emailNotification.Body;

            if (emailNotification.To.ToLower().Contains("shopfehler"))
                mailMessage.Bcc.Add(new MailAddress(_backupBccEmail));

            if (!string.IsNullOrEmpty(emailNotification.CC))
                mailMessage.CC.Add(emailNotification.CC);

            if (!string.IsNullOrEmpty(emailNotification.BCC))
                mailMessage.Bcc.Add(emailNotification.BCC);

            if (!string.IsNullOrEmpty(emailNotification.ReplyToList))
                mailMessage.ReplyToList.Add(emailNotification.ReplyToList);


            if (emailNotification.AttachmentsPaths.Count > 0)
            {
                foreach (var path in emailNotification.AttachmentsPaths)
                {
                    mailMessage.Attachments.Add(new Attachment(path));
                }
            }

            mailMessage.IsBodyHtml = emailNotification.IsBodyHtml;

            return mailMessage;
        }
    }
}

backupBccEmail is required for manipulating the EmailNotification based on some logical conditions

The second question is: When I tried to test the logic of this method It failed because it uses an external resource at the line of adding new attachments where it cannot find the related paths on the Hard Drive, what do you people think about it?

Unit Test Note: I know that only one assert per test is recommended but for now let's keep it simple & straightforward

[TestFixture]
public class EmailMailMessageFactoryTests
{
    private EmailNotification _emailNotification;

[SetUp]
public void SetUp()
{
    _emailNotification = new EmailNotification("from@test.com", "to@test.com;shopfehler@mail.com", "subject", "body", true)
    {
        AttachmentsPaths = new List<string> { "1", "2" },
        CC = "cc@test.com",
        BCC = "bcc@test.com"
    };
}

[Test]
public void CreateMailMessage_WhenCalled_CreatesMailMessage()
{
    var emailMailMessageFactory = new EmailMailMessageFactory("backup@test.com");

    var result = emailMailMessageFactory.CreateMailMessage(_emailNotification);

    Assert.That(result.From.Address, Is.EqualTo(_emailNotification.From));
    Assert.That(result.To[0].Address, Is.EqualTo(_emailNotification.To));
    Assert.That(result.Subject, Is.EqualTo(_emailNotification.Subject));
    Assert.That(result.Body, Is.EqualTo(_emailNotification.Body));
    Assert.That(result.IsBodyHtml, Is.EqualTo(_emailNotification.IsBodyHtml));

    Assert.That(result.Attachments.Count, Is.EqualTo(_emailNotification.AttachmentsPaths.Count));
    //this assert fails "FileNotFoundException"

    Assert.That(result.CC[0].Address, Is.EqualTo(_emailNotification.CC));
    Assert.That(result.Bcc[0].Address, Is.EqualTo(_emailNotification.BCC));
}

}

Maybe I'm misusing what's called Factory Pattern in my code!

thanks in advance

\$\endgroup\$
  • 1
    \$\begingroup\$ Please post sufficient context to help us review your code. Include code for IEmailNotification and redirectToEmail. Also show us how you use this code through a trivial unit test. \$\endgroup\$ – dfhwze Jul 17 at 9:29
  • \$\begingroup\$ and be cautious calling something an abstract factory .. this is a different pattern altogether en.wikipedia.org/wiki/Abstract_factory_pattern \$\endgroup\$ – dfhwze Jul 17 at 9:33
  • \$\begingroup\$ @dfhwze "be cautious calling something an abstract factory" that's why I have doubts about it, I'll edit the question \$\endgroup\$ – Rasheed Jul 17 at 9:38
  • \$\begingroup\$ I couldn't come up with a better title, I hope this is enough! \$\endgroup\$ – Rasheed Jul 17 at 11:05
2
\$\begingroup\$

You are disposing the message before it even has a chance to be used by a consumer.

Remove the using block in the factory method

public override MailMessage CreateMailMessage(IEmailNotification emailNotification) {
    var mailMessage = new MailMessage();
    mailMessage.From = new MailAddress(emailNotification.From);
    mailMessage.To.Add(emailNotification.To);
    mailMessage.Subject = emailNotification.Subject;
    mailMessage.Body = emailNotification.Body;

    if (emailNotification.To.ToLower().Contains("shopfehler"))
        mailMessage.Bcc.Add(new MailAddress(_backupBccEmail));

    if (!string.IsNullOrEmpty(emailNotification.CC))
        mailMessage.CC.Add(emailNotification.CC);

    if (!string.IsNullOrEmpty(emailNotification.BCC))
        mailMessage.Bcc.Add(emailNotification.BCC);

    if (!string.IsNullOrEmpty(emailNotification.ReplyToList))
        mailMessage.ReplyToList.Add(emailNotification.ReplyToList);


    if (emailNotification.AttachmentsPaths.Count > 0) {
        foreach (var path in emailNotification.AttachmentsPaths) {
            mailMessage.Attachments.Add(new Attachment(path));
        }
    }

    mailMessage.IsBodyHtml = emailNotification.IsBodyHtml;

    return mailMessage;        
}

Leave the responsibility of disposal to the consumer of the factory.

my factory method takes a parameter of type IEmailNotification, does that violates the main Job of a Factory which is only creating objects?

A factory method can take explicit dependencies which it can use to perform its required functionality.

When I tried to test the logic of this method It failed because it uses an external resource at the line of adding new attachments where it cannot find the related paths on the Hard Drive, what do you people think about it?

Implementation concerns should be encapsulated behind abstractions that avoid tight coupling to external dependencies.

In this case, when you were testing, the Attachment will try to read the file at the provided path. Since those paths may not exist when testing, you should consider refactoring the current design.

Provide an abstraction that would allow the attachment stream to be read in isolation without any adverse behavior.

public interface IFileInfo {
    string Name { get; }
    string PhysicalPath { get; }
    Stream CreateReadStream();
}

Here is a simple implementation that can be used at run-time

public class AttachmentInfo : IFileInfo {
    private readonly FileInfo innerFile;

    public AttachmentInfo(string path) {
        innerFile = new FileInfo(path);
    }

    public string Name => innerFile.Name;

    public string PhysicalPath => innerFile.FullName;

    public Stream CreateReadStream() => innerFile.OpenRead();
}

The email notification can be refactored to use the abstraction for attachments

public interface IEmailNotification : INotification {
    string From { get; }
    string Subject { get; }
    bool IsBodyHtml { get; }
    string CC { get; }
    string BCC { get; }
    string ReplyToList { get; }
    List<IFileInfo> Attachments { get; }
}

Resulting in the factory method to become

public class EmailMailMessageFactory : MailMessageFactory {

    public EmailMailMessageFactory(string backupBccEmail)
        : base(backupBccEmail) {
    }

    public override MailMessage CreateMailMessage(IEmailNotification emailNotification) {
        var mailMessage = new MailMessage {
            From = new MailAddress(emailNotification.From),
            Subject = emailNotification.Subject,
            Body = emailNotification.Body,
            IsBodyHtml = emailNotification.IsBodyHtml
        };
        mailMessage.To.Add(emailNotification.To);

        if (emailNotification.To.ToLower().Contains("shopfehler"))
            mailMessage.Bcc.Add(new MailAddress(_backupBccEmail));

        if (!string.IsNullOrEmpty(emailNotification.CC))
            mailMessage.CC.Add(emailNotification.CC);

        if (!string.IsNullOrEmpty(emailNotification.BCC))
            mailMessage.Bcc.Add(emailNotification.BCC);

        if (!string.IsNullOrEmpty(emailNotification.ReplyToList))
            mailMessage.ReplyToList.Add(emailNotification.ReplyToList);

        if (emailNotification.Attachments.Count > 0) {
            foreach (var file in emailNotification.Attachments) {
                Stream stream = file.CreateReadStream();
                string filename = file.Name;
                var attachment = new Attachment(stream, filename);
                mailMessage.Attachments.Add(attachment);
            }
        }
        return mailMessage;
    }
}

When testing in isolation, a fake stream can be given to the attachment to allow the subject under test to be exercised.

[TestClass]
public class EmailMailMessageFactoryTests {
    [TestMethod]
    public void CreateMailMessage_WhenCalled_CreatesMailMessage() {
        //Arrange
        var stream = new MemoryStream();
        var attachments = new List<IFileInfo> {
            Mock.Of<IFileInfo>(_ => _.Name == "1" && _.CreateReadStream() == stream)
        };
        var _emailNotification = Mock.Of<IEmailNotification>(_ =>
            _.From == "from@test.com" &&
            _.To == "to@test.com" &&
            _.Subject == "subject" &&
            _.Body == "body" &&
            _.IsBodyHtml == true &&
            _.CC == "cc@test.com" &&
            _.BCC == "bcc@test.com" &&
            _.Attachments == attachments
        );
        var emailMailMessageFactory = new EmailMailMessageFactory("backup@test.com");

        //Act
        MailMessage result = emailMailMessageFactory.CreateMailMessage(_emailNotification);

        //Assert
        result.From.Address.Should().BeEquivalentTo(_emailNotification.From);
        result.To[0].Address.Should().BeEquivalentTo(_emailNotification.To.Split(',')[0]);
        result.Subject.Should().BeEquivalentTo(_emailNotification.Subject);
        result.Body.Should().BeEquivalentTo(_emailNotification.Body);
        result.IsBodyHtml.Should().Be(_emailNotification.IsBodyHtml);

        result.Attachments.Count.Should().Be(_emailNotification.Attachments.Count);

        result.CC[0].Address.Should().BeEquivalentTo(_emailNotification.CC);
        result.Bcc[0].Address.Should().BeEquivalentTo(_emailNotification.BCC);
    }
}
\$\endgroup\$
  • \$\begingroup\$ I'm really grateful for your time & effort! yesterday I tried a similar way to let the test pass by introducing a new interface IAttachmentProvider where a concrete implementation should define a method called GetAttachment similar to DateProvider.GetNow() and then in the test I mocked this interface and used the attachment constructor which accepts Stream & FileName and that let the test pass, so now I'm also confident that the pattern I have is a real factory. thanks again : ) \$\endgroup\$ – Rasheed Jul 18 at 10:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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