7
\$\begingroup\$

I've come from a PHP background and I'm really trying to unlearn the bad habits I acquired from not doing things properly with PHP through my own lack of understanding..

I'm looking to advance my understanding and implementation of OOP principles. Below I have a simple email notifier class. I have a few ideas of how I could do this better. I would like your guys advice as well. First I believe I would take the Notifier constructor and pass in both the Port & Host information. When I instantiate the class I would pass these values in.

Both the generateRequestNotification & generateUserNotification methods could be broken down into smaller pieces I believe as well...

Then maybe I could create a static method that would generate 'Body' of the email. Other than that though I'm kind of at a loss of ways to improve this code, though I'm quite certain it's in need of refactoring. Currently the code is operating correctly but I think there could be improvement.

public class Notifier
 {
    private SmtpClient _client = new SmtpClient();
    private List<UserManager> _toManagers;
    private User _fromUser;
    private PersonnelManagementEntities personnel = new    PersonnelManagementEntities();
    public Notifier()
    {
        _client.Port = 25;
        _client.DeliveryMethod = SmtpDeliveryMethod.Network;
        _client.UseDefaultCredentials = false;
        _client.Host = "mail.xxxxxx.com";
    }

    public void sendNotificationToManager(TimeRequest trFromUser)
    {
        generateRequestNotification(trFromUser);
    }

    public void sendNotificationToUser(TimeRequest trFromManager)
    {
        generateUserNotification(trFromManager);
    }


    //Fetch all Managers associated with user
    private List<UserManager> findManagerIDs(int x)
    {
        var managerList = personnel.UserManagers.Where(u => u.UserID == x).ToList();
        return managerList;
    }

    //Fetch User object associated with TimeRequest
    private User findUser(int x)
    {   
        return personnel.Users.Where(u => u.UserID == x).FirstOrDefault();
    }

    //Create the notification
    private void generateRequestNotification(TimeRequest tr)
    {
        _toManagers = findManagerIDs(tr.EmployeeUserID);
        _fromUser = findUser(tr.EmployeeUserID);

        foreach (UserManager manager in _toManagers)
        {
            string body = string.Format(@"User: {0} has submitted a time off request in TimeTracks.
                                          Start Date: {1}
                                          End Date: {2}
                                          Comment: {3}
                                          Please login to review the request.
                                          Thank you.
                                          http://timetracks.xxxxxxx.com", _fromUser.ADUserName, tr.sDateTime, tr.eDateTime, tr.Comment);


            var toManager = personnel.Users.Where(x => x.UserID == manager.ManagerID).FirstOrDefault();

            MailMessage email_message = new MailMessage(_fromUser.email ?? _fromUser.ADUserName + "@xxxxxxxx.com", toManager.email);
            email_message.Subject = "TimeTracks Notification: " + _fromUser.FullName + " has submitted a request";
            email_message.Body = body;
            _client.Send(email_message);
        }
    }

    private void generateUserNotification(TimeRequest tr)
    {
        string managerResult = ((tr.ManagerApproval == true) ? "approved" : "denied");

        _fromUser = findUser(tr.EmployeeUserID);
        string body = string.Format(@"Your time request for {0} to {1} has been {2}.
                                          Please login to review the request.
                                          Thank you.
                                          http://timetracks.xxxxxxx.com", tr.sDateTime, tr.eDateTime, managerResult);

            var toManager = personnel.Users.Where(x => x.UserID == tr.ManagerID).FirstOrDefault();
            MailMessage email_message = new MailMessage(toManager.email, _fromUser.email);
            email_message.Subject = "TimeTracks Notification: " + toManager.FullName + " has submitted a response to your request. TimeRequest Status: " + tr.ManagerApproval ;
            email_message.Body = body;
            _client.Send(email_message);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ A big one nobody's mentioned yet is that since SmtpClient implements the IDisposable interface, your class should also do so and your Dispose() method should call the SmtpClient's Dispose() method. It's possible that PersonnelManagementEntities is the same way; I do not know for certain. Google for "Disposable pattern C#" and it'll steer you the right way. \$\endgroup\$ – Jesse C. Slicer Apr 25 '16 at 14:52
  • 1
    \$\begingroup\$ 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\$ – Simon Forsberg Apr 25 '16 at 17:28
  • \$\begingroup\$ @SimonForsberg ok my mistake. I will post a follow up answer with descriptions of what a changed and give credit to the members who helped me. \$\endgroup\$ – Waragi Apr 25 '16 at 17:32
8
\$\begingroup\$

I believe I would take the Notifier constructor and pass in both the Port & Host information. When I instantiate the class I would pass these values in.

Good idea.

Both the generateRequestNotification & generateUserNotification methods could be broken down into smaller pieces I believe as well...

Agreed again.

I would extract the inner part of the loop in generateRequestNotification (foreach (UserManager manager in _toManagers)). Whenever there's a loop wrapped around a large block of code, it smells of violation of Single Responsibility Principle to me. Because iterating through entities seems to be sort of a responsibility in its own right, separate from doing whatever the body of the loop is doing with them.

Also the code that handles email_message in both these methods is repetitive and could be encapsulated in a separate, parameterized method.

Then maybe I could create a static method that would generate 'Body' of the email.

It doesn't have to be static, although there are conflicting schools of thought here. On one hand, making a method static sort of stresses the fact that it's stateless - and for instance the popular code quality tool for C#, ReSharper, suggests this by default.

On the other hand, making a method static has certain consequences, such as an inability to override the method. I don't think it's a big deal either way in this case, anyhow it's a little controversial whether statics should be used by default.

As for other remarks:

Overall design

The class is called Notifier, but it knows some other tricks beyond notifying (by sending emails. By the way, I would consider renaming it to EmailNotifier to resolve any possible ambiguity, since there are various types of notifications in this world, but I'm not hellbent on it).

What it also does is that it searches personnel members by their ID - that doesn't qualify as "notifying" in my book? I would expect retrieving a manager by ID to be implemented on PersonnelManagementEntities - let every object take care of itself (aka Law of Demeter).

Naming

  • In C# the convention is that methods are named with upper-case, just like classes.

  • Pascal/camel case is adviced instead of underscore (email_message), even more so as you're not consistent with it (email_message, but right next to it you've got toManager, not to_manager).

  • Names should be descriptive - the parameter for findUser shouldn't be called x if it represents an id.

  • Don't use overly terse names, such as TimeRequest tr, whether as standalone words or Hungarian prefixes (TimeRequest trFromManager). Both are at odds with .NET code style. I'd rather make it TimeRequest timeRequest and TimeRequest fromManager, respectively.

  • If you are using the underscore prefix for fields (some frown upon it, but it's a matter of taste), be consistent about it - why is it _client, but personnel?

Code structure

  • I can't see the purpose of your public methods sendNotification and sendNotificationToUser, given all they do is redirect the call to (private) generate... methods with the same signature. Why not just inline the implementation from private methods?

    This facade serves no purpose other than adding noise, and needlessly doubling the vocabulary - what's the difference between "sending" and "generating", exactly?

  • There is no reason for _toManagers and _fromUser to be fields rather than local variables (or, taking it one step further, not to exist at all). Their lifespan doesn't go beyond a single method call.

Cosmetics

  • ((tr.ManagerApproval == true) ? "approved" : "denied") - this is probably your PHP background showing through, in C# there's no need for this == true bit. And while we're at it, the enclosing parentheses are redundant, too. It could be simplified to tr.ManagerApproval ? "approved" : "denied".

  • return personnel.Users.Where(u => u.UserID == x).FirstOrDefault();

    Even .NET devs often forget that FirstOrDefault takes an optional predicate argument, so it can be simplified by cutting the Where call out:

    return personnel.Users.FirstOrDefault(u => u.UserID == x);

    Note that you are reinventing this in generateRequestNotification:

    var toManager = personnel.Users.Where(x => x.UserID == manager.ManagerID).FirstOrDefault();

    It's better to reuse findUser there.

  • //Fetch User object associated with TimeRequest

    This comment is wrong, or confusing, because you're not passing a TimeRequest instance to findUser method, so I can't see where that association is.

  • Use XML documentation comments for methods:

    /// <summary>
    /// Fetch User object associated with TimeRequest
    /// </summary>
    

    Visual Studio will autocomplete the template for you once you type "///" and press enter, if I recall correctly. One benefit of that is that it allows to feed the info to documentation generating tools, but more importantly, xml comments come up in IntelliSense preview.

  • "TimeTracks Notification: " + toManager.FullName + " has submitted a response to your request. TimeRequest Status: " + tr.ManagerApproval

    You are using String.Format in some other cases, so why not here? Also note that if you're on C# 6.0, you have interpolated strings at your disposal. It's just syntax sugar for String.Format - helps to improve readability.

  • Immutable fields - or fields that are initialized once and are not meant to ever get overwritten - can be marked as readonly to stress the intention (that would be personnel and client).

Error handling

or the lack thereof. Sending an email can fail for a variety of reasons, and you don't handle exceptions or failures.

In generateRequestNotifications you're sending emails in a loop - what if there's 10 managers, and sending it to the second one fails badly, shouldn't we try to ensure that the remaining eight would still receive their notifications?

You don't handle edge cases either (such as an inexistent recipient: it would result in a NullReferenceException once you tried to retrieve email from a null fromUser, if personnel didn't contain any entity under a given id).

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you so much, I have a lot of material to go through in your post. I greatly appreciate you taking the time to go through my class. I will begin to refactor my code and be more vigilant with my naming conventions. \$\endgroup\$ – Waragi Apr 25 '16 at 14:44
  • \$\begingroup\$ No problem @Waragi, glad I could be of help :) \$\endgroup\$ – Konrad Morawski Apr 25 '16 at 14:45
3
\$\begingroup\$

You can configure SmtpClient in your app.config with the following configuration:

<system.net>
  <mailSettings>
    <smtp from="sender@example.com">
      <network enableSsl="true" host="smtp.example.com" port="587" userName="smtpusername" password="smtppassword" />
    </smtp>
  </mailSettings>
</system.net>

Then in its simplest form, I'd just send a message with: (Of course, if you don't send both text and html versions you just omit one AlternateView)

var message = new MailMessage { Subject = subjectOfMessage };
message.To.Add(new MailAddress(recipientAddress, recipientName));

var htmlView = AlternateView.CreateAlternateViewFromString(bodyInHtml, null, "text/html");
message.AlternateViews.Add(htmlView);

var textView = AlternateView.CreateAlternateViewFromString(bodyInPlainText);
message.AlternateViews.Add(textView);

new SmtpClient().Send(message);

You may also want to look at RazorEngine for generating custom html messages.

\$\endgroup\$
  • \$\begingroup\$ I would explain why it may be a better solution than OP's current, because it seems a bit overkill at least to me. \$\endgroup\$ – edmz Apr 25 '16 at 19:49
  • \$\begingroup\$ @black you mean regarding RazorEngine? Yeah, that's what the may was for. Jsut thought it was a relevant package to know about. \$\endgroup\$ – Mattias Åslund Apr 25 '16 at 20:04
1
\$\begingroup\$

Some quick remarks:

  • Method names need to be PascalCase.
  • Do not use uncommon abbreviations, e.g. "tr" in trFromUser.
  • Do not use meaningless parameter names like x.
  • Make sure your method names do what they're named for: findManagerIDs for instance returns a List<UserManager>, whereas I'd expect a collection of ints.

Embedding the email message in your code isn't what I would recommend: I'd consider moving those to a template, for instance.


What's the point of _toManagers and _fromUser being class-level fields?

\$\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.