Registering to "this" object's own event handler within "this" method.

Question

I would like to provide a deafult implementation for an event generated by an instance of “Authenticator” (hereafter “objAuthenticator”).

Is this an acceptable practice to provide such method that registers for handling objAuthenticator’s own event to provide a deafult implementation?

And also, if it is an acceptable pratice, I have two follow-up questions. I have two overloaded methods for “SendEmailOnAuthenticationFailed”.

• Which one should I expose to the outside world?

• Which one would cause less coupling between classes?

  public class Authenticator
  {
          public event EventHandler AuthenticationFailed = delegate { };
          protected virtual void OnAuthenticationFailed()
          {
              var handler = AuthenticationFailed;
              handler(this, EventArgs.Empty);
          }
  
  
      public void IsAuthenticated()
      {
          // Some logic...
          // ...
          // Woops authentication failed
          OnAuthenticationFailed();
      }
  
  
      // Is this a better option?
      public void SendEmailOnAuthenticationFailed()
      {
          SendEmailOnAuthenticationFailed(EmailSender);
      }
  
  
      // Or is this?
      public void SendEmailOnAuthenticationFailed(EventHandler emailSender)
      {
          AuthenticationFailed += emailSender;
      }
  
  
      private void EmailSender(object sender, EventArgs e)
      {
          Console.WriteLine("Send email: EmailSender");
      }
  }
  
  
  

[UPDATE]: Answer to Marc Gravell's question

I'm very confused about what your code is trying to do

So far, "SendEmailOnAuthenticationFailed" is not exposed to the outside world. What I am trying to do is that (I just added IsAuthenticated) within "IsAuthenticated", when an authentication fails, I would like to send an email without having to write the same event handler everywhere.

[UPDATE2]: I have realized that there is no need to expose "AuthenticationFailed". I will keep the class as closed as possible without exposing the event.

Why can't we mark more than one answers as "answer"? ...

[UPDATE3]: Final output: Here is the how I decided to implement.

public class Authenticator
{
    private readonly IEmailResponder _EmailResponder;

    public Authenticator(IEmailResponder emailResponder)
    {
        _EmailResponder = emailResponder;
    }

    public void IsAuthenticated()
    {
        // Some logic...
        // ...

        // Woops authentication failed
        SendEmailForAuthenticationFailure();
    }

    private void SendEmailForAuthenticationFailure()
    {
        _EmailResponder.SendEmail(...);
    }
}

Answer 1

It is unusual to listen to your own events; such logic can typically go into the OnAuthenticationFailed method. That said - I'm very confused about what your code is trying to do, which makes it a little hard to answer...

I doubt it applies, but note additionally that there are some subtle synchronisation issues you might need to think about (when talking to your own events) if this code is highly threaded (which most isn't).

Answer 2

I think I misunderstood your question first time... am I right in saying you want an optional way of doing this? Personally I suspect I'd make that an option in the constructor:

public class Authenticator
{
    public event EventHandler AuthenticationFailed = delegate { };

    public Authenticator(bool sendEmailOnFailure)
    {
         if (sendEmailOnFailure)
         {
             // No need for the no-op delegate any more, so just replace it.
             AuthenticationFailed = EmailSender;
         }
    }
}

Alternatively, separate out the "email sending" functionality from the "authentication" functionality for better separation of concerns, and let the caller subscribe using an email handler if they want to. That's probably a neater solution.

Answer 3

The answer to the first question is that it is acceptable. For your follow-ups, the answer is it depends. The first one internalizes the email process. If you choose to do this, make sure that you're injecting the email logic. If you choose the latter, it allows you to inject the behavior. If you do the second one, however, I would suggest that you change the name, since it doesn't matter if it's email, or MQ, or tiddlywinks. In fact, the second method isn't really needed, since they can subscribe directly to the event.

If you're concerned about decoupling, you need to think about the single responsibility of the Authenticator class. It should be to authenticate, not to send emails. Try this:

public class Authenticator
{
    public event EventHandler AuthenticationFailed = delegate { };

    protected virtual void OnAuthenticationFailed()
    {
        AuthenticationFailed(this, EventArgs.Empty);
    }
}

now, consume authenticator:

AuthenticationEmailResponder responder = new AuthenticationEmailResponder(emailAddress, emailServer);
objAuthenticator.AuthenticationFailed += responder.SendFailureMessage;