current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

Requirement:

If the bookshop manager field changes from A to B, I need to email the person B saying that the new Bookshop has been allotted to him and he has to do certain activities.

Here is my method

private void SendEmailToAllottedManager()
{
   EmailExpress mEmailExpress = new EmailExpress();
   mEmailExpress.SendEmail(Shop.Bookstore.BookstoreName, Subject);
}

In the method where I am calling SendEmailToAllottedManager, I am checking if the Manager has indeed changed or not as below - 

if (OriginalManager != CurrentManager)
{
    SendEmailToAllottedManager();
}

During code review, a colleague suggested that I move the check from outside the function SendEmailToAllottedManager() to inside it.

Proposed Code

private void SendEmailToAllottedManager()
{
    if (OriginalManager != CurrentManager)
    {
          EmailExpress mEmailExpress = new EmailExpress();
          mEmailExpress.SendEmail(Shop.Bookstore.BookstoreName, Subject);
    }

}

I maintain my stand that the function SendEmailToAllottedManager is only about emailing and not about performing the checks etc.,

What do you think?

share|improve this question
1  
SendEmailToManagerIfRequired –  Grant Thomas Jul 15 '11 at 12:28
    
Mr Disappointment: Sorry...could not understand what you are trying to convey here. If you mean to say that SendEmailToManagerIfRequired is the place for the check to go, can you please elaborate as to why? –  Kanini Jul 15 '11 at 13:25
    
I was just expressing that there is always the option to rename the method to include the fact that it will only send the email if required (i.e. post checking.) –  Grant Thomas Jul 15 '11 at 14:52
add comment

2 Answers

up vote 3 down vote

I would say, that actually sending the email is part of the process of changing the store manager. It's part of the business logic (AFAIK), that when a store changes it's manager, the new manager should be notified, among other things.

Thats why the check if the manager changes is not a job for the email sender at all. The email sender should be a utility, which can be reused.

share|improve this answer
add comment
up vote 1 down vote

I agree with you. The performed action of a method should correspond to it's name. In your colleague's suggestion, an email sent to the allotted manager is, as you already pointed out, not always being sent and thus not 100% semantically accurate.

However, for the sake of readability, it sometimes can be easier to put the if-then check in the same method, especially if you have a lot of if-then checks to be performed. But then you might want to consider doing some refactoring instead, if it becomes too bloated. It's always hard to know where to draw the line.

Interesting question! Im looking forward to seeing others thoughts.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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