C# SMTP notifier client

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 _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 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.Us

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:

/// 
/// Fetch User object associated with TimeRequest
/// 

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 Notificat

StackExchange Code Review Q#126645, answer score: 8