14
\$\begingroup\$

I have been looking into stuff like method chaining, Cascade-Lambda pattern etc.

I have created a class which seems to work fine. However, I just wanted to confirm if this is the best way and the right way to implement this.

For the cascade lambda pattern I was unable to use Action action as a parameter to the Send() method. I couldn't figure out how would I use it.

Any suggestions are welcome.

Here is the code:

public class MailManager
{
    MailMessage _mail { get; set; }
    SmtpClient _smtp { get; set; }

    public MailManager()
    {
        _mail = new MailMessage();
        _smtp = new SmtpClient();
    }

    public MailManager To(string address)
    {
        _mail.From = new MailAddress(address);
        return this;
    }

    public MailManager From(string address)
    {
        _mail.To.Add(new MailAddress(address));
        return this;
    }

    public MailManager Subject(string subject)
    {
        _mail.Subject = subject;
        return this;
    }

    public MailManager Body(string body)
    {
        _mail.Body = body;
        return this;
    }

    public MailManager IsBodyHtml(bool isBodyHtml = true)
    {
        _mail.IsBodyHtml = isBodyHtml;
        return this;
    }

    public MailManager AlternateViews(AlternateView alternateView)
    {
        _mail.AlternateViews.Add(alternateView);
        return this;
    }

    public MailManager Host(string host)
    {
        _smtp.Host = host; return this;
    }

    public MailManager Port(int port)
    {
        _smtp.Port = port; return this;
    }

    public MailManager Credentials(NetworkCredential credentials)
    {
        _smtp.Credentials = credentials; return this;
    }

    public MailManager EnableSsl(bool enableSsl = true)
    {
        _smtp.EnableSsl = enableSsl; return this;
    }        

    public void Send()
    {
        using (_smtp)
        {
            _smtp.Send(_mail);
        }
    }
}

This is how I use it:

mailManager.From(_emailSettings.Email)
           .To(item.Email)
           .Subject(string.Format(_emailSettings.Subject, item.Name)).AlternateViews(htmlView)
           .Body(body).IsBodyHtml()
           .Host(_emailSettings.Domain)
           .Port(_emailSettings.Port)
           .Credentials(new NetworkCredential(_emailSettings.Email, _emailSettings.Password))
           .EnableSsl()
           .Send(); 
\$\endgroup\$
4
  • 5
    \$\begingroup\$ I have rolled back your edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Mar 28 '18 at 11:26
  • 2
    \$\begingroup\$ When you build an API that's similar to this - never mutate, methods like To and From should return new instances, the cost isn't too big and the bugs hen people make assumptions about it are terrible - that's why things like DateTime or strings don't mutate but return DateTimes (or strings). \$\endgroup\$ – Benjamin Gruenbaum Mar 28 '18 at 13:17
  • 2
    \$\begingroup\$ There are some reasons to use this sort of pattern but none of them are represented here. The object remains externally mutable, each method returns an object of the same type, and no attempt is made to guide consumers to correct usage. Given that I would say that this code is a waste. \$\endgroup\$ – Aluan Haddad Mar 28 '18 at 23:26
  • 2
    \$\begingroup\$ That doesn't look like a MailManager to me - it doesn't manage any mail. It's more like an EmailBuilder. \$\endgroup\$ – Bergi Mar 29 '18 at 1:18
13
\$\begingroup\$

As far as method chaining goes, this is neatly implemented.


You've swapped To and From, the method names are inverted. I assume that's just a typo.

public MailManager To(string address)
{
    _mail.From = new MailAddress(address);
    return this;
}

public MailManager From(string address)
{
    _mail.To.Add(new MailAddress(address));
    return this;
}

As it currently stands, I don't think method chaining is really needed here. Every method operates as a basic property setter, which can be handled by using public properties.

This is a matter of preference; but in C# I'd be more inclined to use public properties as opposed to methods.
In the past, all we had was fields and methods. When the tiniest amount of logic was needed to set a field value, we had to rely on methods. Properties were added as a compromise: they are used for setting field values, but you're able to add some minor logic (within reason).

Simple string values can be handled by basic properties:

public string Subject
{
    get { return _mail.Subject; }
    set { _mail.Subject = value; }
}

Note: This example directly sets the value of _mail.Subject. Personally, I'd prefer to not use _mail until the Send() was triggered (and store the values in properties until then), but that may be too subjective for a review.

Your current To() method somewhat conceals the fact that you can add more than one recipient. It doesn't suggest that you're adding to a list. Using a property, this is slightly easier to understand:

public List<string> To { get; set; }

You can cast these into the appropriate MailAddress object and add it to _mail inside the Send() method:

_mail.To.AddRange(this.To.Select(s => new MailAddress(s));

An example of how you'd use a property-based approach:

var mailManager = new MailManager() {
    From = _emailSettings.Email,
    To = new List<string>() { item.Email },
    Subject = string.Format(_emailSettings.Subject, item.Name),
    Body = body,
    IsBodyHtml = true,
    Host = _emailSettings.Domain,
    Port = _emailSettings.Port,
    Credentials =new NetworkCredential(_emailSettings.Email, _emailSettings.Password),
    EnableSsl = true
};

mailManager.Send(); 

Note: You can also inline everything, i.e; (new MailManager() { ... }).Send();.

Note: I set the property values in the property initializer. You're not required to do so, you can also just store the MailManager in a variable and set its properties when you want to at a later stage (but obviously before calling Send()).

Should you avoid method chaining?
No. You wanted to use it, and you used it in a good way. I'm just trying to offer you an alternative which requires less code to achieve the same result.

If you were implementing method chaining in a class where there is a lot of business logic involved when a method gets calls, then method chaining becomes the preferred approach.
For the current MailManager class, I find that it doesn't have enough business logic to warrant method chaining just yet. But it's not particularly wrong to do so, just a bit more cumbersome (in my opinion).


Some minor nitpicks:

  • If you're going to use method chaining to simplify the syntax, it seems consistent to change the Credentials method to take two strings (email,password) instead of a NetworkCredential, or at least create two overloaded methods to allow for either option. Credentials(_emailSettings.Email, _emailSettings.Password) is nicer to read than Credentials(new NetworkCredential(_emailSettings.Email, _emailSettings.Password))
  • IsBodyHtml() Based on its name, this should be a method that returns a boolean, instead of setting a boolean. A better name would be SetBodyHtml()
  • AlternateViews(AlternateView alternateView) should really be called AddAlternateView(AlternateView alternateView)
  • Similarly, To really should be called AddRecipient, since calling the method adds a second value instead of overwriting the first value.
    • I chose AddRecipient instead of AddTo to avoid semantical confusion between "Add a To-recipient" and "Add this to that".
  • I would personally change EnableSsl(bool enableSsl = true), though I can see argument for why you used your version.
    • UseSsl(bool useSsl = true)
    • EnableSsl() and DisableSsl()
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the inputs. I have updated the code. Could you please check it and share your views \$\endgroup\$ – Praneet Nadkar Mar 28 '18 at 11:24
6
\$\begingroup\$

This looks suspicious

public MailManager To(string address)
{
    _mail.From = new MailAddress(address);
    return this;
}

public MailManager From(string address)
{
    _mail.To.Add(new MailAddress(address));
    return this;
}  

To is adding to From and Fromis adding to To.


You aren't consistent in your coding style. Something like _smtp.Credentials = credentials; return this; is a big no-go because it reduces the readability of the code. In addition you use a style like this

public MailManager AlternateViews(AlternateView alternateView)
{
    _mail.AlternateViews.Add(alternateView);
    return this;
}  

as well.


Although you are using a using with _smtp which is usually good to do, it creates the need, if another mail should be sent, to create a new MailManager object because _smtp will be disposed after the mail is sent.

\$\endgroup\$
1
  • \$\begingroup\$ with regard to your final comment, I would suggest making MailManager implement IDisposable and the Dispose() method calls Dispose() on both _smtp and _mail. Then remove the using in that class and let the caller wrap it. \$\endgroup\$ – Jesse C. Slicer Mar 28 '18 at 19:02
6
\$\begingroup\$

@Flater and @Heslacher already mentioned most issues so I'll throw in just one more comment...

What you've implemented is very similar to the Builder pattern and Fluent interface.

In your case however this isn't very useful because it doesn't really add any new functionality, abstractions and also does not make anything simpler. It just delegates all calls to actual underlying objects.

I also support the optinion that this is not the right way and that the object initializer is in this case sufficient.

If you would have introduced any abstractions for the email-client or the email-message then a builder might be a better choice but not this time.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the inputs. I have updated the code. Could you please check it and share your views \$\endgroup\$ – Praneet Nadkar Mar 28 '18 at 11:25
5
\$\begingroup\$

Most answers address the majority of the issues. However, I don't see anyone addressing one important problem: testability.

If you want to write unit tests that will verify your MailManager is working as expected, you'll need to be able to mock your dependencies the class being tested relies upon. In other words, for example, you don't want to send a real email, so you'd need to mock or use a fake SMTP client.

In order to achieve this, you could do something like:

public class MailManager
{
    private readonly IMailMessage _mailMessage;
    private readonly ISmtpClient _smtpClient;

    public MailManager(IMailMessage mailMessage, ISmtpClient smtpClient)
    {
        _mailMessage = mailMessage;
        _smtpClient = smtpClient;
    }
    ...
}

And then inject the implementation you want the MailManager to use.

\$\endgroup\$
0
\$\begingroup\$

It would be nice if values could also be passed in the ctor.

Why do you need { get; set; } on _mailMessage and _smtpClient.

You don't test for required values before calling _smtp.Send(_mail);

\$\endgroup\$
0
\$\begingroup\$

Looks like you have complicated things bit.

  1. Avoid patterns if functionalities can be achieved through simple properties, i think Fluent interface pattern is used.

  2. MailManagaer should implement IValidate interface, to check the validity of the mail before sending mail

     public interface IValidate
     {
         bool IsValid();
     }
    
  3. MailManager should interface IMailManager interface for sending the message

    public interface IMailManager
    {
         void Send()
    }
    
  4. We can extend the functionalities by create new extension methods on IMailManager

     public static class MailManagerExtensions
     {
          public static IMailManager Draft(this IMailManager mailManager, ....)
          {
          }
     }
    
  5. Create overloaded constructors for MailManager, and expose properties

     public MailManager(string fromAddress, List<string> toAddresses, string subject)
     public MailManager(string fromAddress, List<string> toAddresses, string subject, ....)
    
  6. Usage of the class is as follows

    IMailManager mailManager = new MailManager(.....);
    mailManager.send();
    
\$\endgroup\$

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.