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);    }}
C# SMTP notifier client
c#;email
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 designThe 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).NamingIn 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 structureI 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 TimeRequestThis 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.ManagerApprovalYou 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 handlingor 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). 