1
\$\begingroup\$

I have an ASP.net MVC5 project which involve different domain services, and one of it are EmailService which will send email if entity is updated.

public class Project {
    string ProjectType {get; set;}
    string Name {get; set;}
}

public interface IProjectService {
    event void EventHandler<ProjectEventArgs> Updated;
}

public interface IEmailLogRepository {
    void Insert(EmailLog obj);
}


public class EmailService {

    private readonly IEmailLogRepository _repos;

    public string SmtpServer {get;set;}
    public string SmtpPort {get;set;}
    public string SmtpSender {get;set;}
    public string SmtpUsername {get;set;}
    public string SmtpPassword {get;set;}
    ......

    public string TemplateASubjectFormat {get;set;}
    public string TemplateBSubjectFormat {get;set;}
    public string TemplateAContentFormat {get;set;}
    public string TemplateBContentFormat {get;set;}

    public EmailService(IProjectService service, IEmailLogRepository repos) {
        service.Updated += OnProjectUpdated;
        _repos = repos;
    }

    private void OnProjectUpdated(object sender, ProjectEventArgs args) {
        if (args.Entity.ProjectType == "A")  SendEmailWithTemplateA(args.Entity);
        if (args.Entity.ProjectType == "B")  SendEmailWithTemplateB(args.Entity);
    } 

    private void SendEmailWithTemplateA(Project obj){
        MailMessage mail = new MailMessage();
        mail.Suject = string.Format(TemplateASubjectFormat, obj.Name);
        mail.Content = string.Format(TemplateAContentFormat, obj.Name);
        ......
        SendEmail(mail);
    }

    private void SendEmailWithTemplateB(Project obj){
        MailMessage mail = new MailMessage();
        mail.Suject = string.Format(TemplateBSubjectFormat, obj.Name);
        mail.Content = string.Format(TemplateBContentFormat, obj.Name);
        ......
        SendEmail(mail);
    }

    private void SendEmail(MailMessage mail){

        EmailLog log = new EmailLog();
        log.Email = mail.ToAdress;

        try{
            SmtpClient smtp = new SmtpClient();
            ........
            smtp.Send(mail);
            log.Status = "Success";
            _repos.Insert(log);
        }catch(Exception ex){
            log.Status = string.Format("Error: {0}", ex.Message);
            _repos.Insert(log);
        }
    }
}

public class ProjectController : Controller
{
    private readonly IProjectService _service;
    private readonly EmailService _emailService;

    public ProjectController(IDbConnection connection, IProjectService service, IEmailLogRepository emailRepos){
        _service = service;
        _emailService = new EmailService(_service, emailRepos);
        Dictionary<string, string> settings = Settings.Load(connection);
        _emailService.SmtpServer = settings["SmtpServer"];
        ........
        _emailService.TemplateASubjectFormat = settings["TemplateASubjectFormat"];
        _emailService.TemplateAContentFormat = settings["TemplateAContentFormat"];
        _emailService.TemplateBSubjectFormat = settings["TemplateBSubjectFormat"];
        _emailService.TemplateBContentFormat = settings["TemplateBContentFormat"];
        ........
    }
}

In the above code, the EmailService contains lots of settings like Smtp and template info for different project types, those settings are located in database currently (one settings table but it stored all settings for different services), and the service properties are assigned in ASP.net controller constructor.

This kind of setting properties assignment caused the constructor contains a lots of lines, some controller may require more than one service to work, each services in my projects are designed like this, and I feel it's not a good design, so may I check what's the best practice for assign settings value for setting assignment?

\$\endgroup\$

closed as off-topic by t3chb0t, esote, Mast, Heslacher, pacmaninbw Apr 18 at 16:02

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – t3chb0t, esote, Mast, Heslacher, pacmaninbw
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Why are there so many ........? You have to post all the code. \$\endgroup\$ – t3chb0t Apr 17 at 18:50
  • \$\begingroup\$ There is a lot of code missing here, making us guess at your intentions. That's not acceptable. Please take a look at the FAQ on how to get the best value out of Code Review. \$\endgroup\$ – Mast Apr 18 at 0:37
  • \$\begingroup\$ Well, I think that the missing parts are somehow straightforward (just repeat for the missing fields) and they may be even removed all together but... \$\endgroup\$ – Adriano Repetti Apr 18 at 7:58
1
\$\begingroup\$

The very first problem is to move some responsibilities out of the controller. IMHO controllers should do as little as possible because their responsibility should be to glue the HTTP request to the domain model.

In your case the controller should not create and configure the service also because it makes impossible (OK...just hard) to test both in isolation. Please pick a better name but the very first thing to do is to receive an IEmailService service exactly the same way you receive the others.

This MIGHT not be always possible without some major refactoring then I'd start introducing a factory:

public ProjectController(IProjectService service, IEmailServiceFactory factory) {
   _service = service;
   _emailService = factory.GetService();
}

That hypothetical factory method is simply:

public GetService() {
    var emailService = new EmailService(_service, emailRepos);
    var settings = Settings.Load(connection);
    emailService.SmtpServer = settings["SmtpServer"];

    // ...
    emailService.TemplateASubjectFormat = settings["TemplateASubjectFormat"];
    // ...

    return emailService;
} 

This is easy because you're just moving your code from one class to another and you introduced another service.

Now controllers came back to a reasonable level of responsibilities (and complexity) and they're also testable because you can mock the IEmailServiceFactory service (no need to send tons of e-mails when running your tests on the controller...)

As soon as you're ready (or simply as a second step on your refactoring) you should receive the IEmaiLService service directly - instead of going trough a factory.


There are also few things to say about the other code. Those initialization lines are tedious, if you're OK to tie your EmailService to its DB representation then you can decorate its properties:

[ConfigurationColumName("SmptServer")]
public string SmtpServer {get;set;}

You can then use a touch of Reflection to enumerate its properties:

foreach (var property in typeof(EMailService).GetProperties()) {
    var attribute = property.GetCustomAttribute<ConfigurationColumNameAttribute>();
    if (attribute == null) {
        continue;
    }

    property.SetValue(obj, settings[attribute.Name]);
}

Of course you need to define a ConfigurationColumNameAttribute class and add proper error handling.


We then arrive to the second error-prone (and not scalable) part of your code. You hard-coded the number of templates. What if you will ever need 3? Or 4? Or just one? You may simply use a dictionary (if not an array):

public Dictionary<string, TemplateDefinition> Templates { get; } = new ...;

Where TemplateDefinition has Subject and Content properties. Code should now be simple enough that you may even avoid Reflection.

SendEmailWithTemplateA() and SendEmailWithTemplateB() are almost exact duplicates, if you use a dictionary then there should not be any repeated code.


Mechanism you're using for templating is fragile. It's hard to write long strings using {0} and {1} as placeholders. Do yourself a BIG favor and switch to a small, fast and well-tested templating engine like Mustache. You'll be able to write your template as Hello {{username}}, we write to you to inform you about the new {{product}}.... It'll be much easier to configure and edit your templates and you'll minimize the chances to mix-up things when changing something in your code. Do not forget that this has to be tested too.


Do not catch Exception, is OutOfMemoryException or AccessViolationException something you want to handle that way?


\$\endgroup\$

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