5
\$\begingroup\$

I'm trying to create a generic notification service that works with

  1. Mail (smtp )
  2. Sms (Third part vendor)
  3. Push events (For future)

Purpose : Create a notification service and it should be pluggable

A Common interface for all notification service

public interface INotificationService
{
    bool Notify();
}

Mail Service

public class MailService : INotificationService
{
    readonly string address;
    readonly string subject;
    readonly string message;

    public MailService(string adrss, string subject, string mesg)
    {
        this.address = adrss;
        this.subject = subject;
        this.message = mesg;
    }

    public bool Notify()
    {
        //pseudo code
        Console.WriteLine("Email service invoked " + address + subject + message);
        return true;

    }
}

Sms Service

public class SmsService : INotificationService
{
    readonly string phoneNumber;
    readonly string message;
    readonly ISmsSender sender;
    public SmsService(string phone, string msg, ISmsSender send)
    {
        this.phoneNumber = phone;
        this.message = msg;
        this.sender = send;
    }

    public bool Notify()
    {
        Console.WriteLine("Sms service invoked" + phoneNumber, message);
        return sender.SendSms(message);

    }
}

ISms Sender interface Mocking third party sms service

public interface ISmsSender
{
    bool SendSms(string msg);
}

public class SmsSender : ISmsSender
{
    public bool SendSms(string msg)
    {
        return true;
    }
}

Consuming program

 INotificationService smsNotification = new 
                                       SmsService("12346789", "content" ,new SmsSender());
smsNotification.Notify();


INotificationService mailNotification = 
                                   new MailService("mail@gmail.com", "subject", "content");
mailNotification.Notify();

Please find the source code and have added couple of unit test here

Questions

  1. Is it a good design or Any better design
  2. Based on the code coverage still 42% are not covered.
  3. Passing all necessary arguments in the constructor have any impact on maintainability
\$\endgroup\$
4
\$\begingroup\$

In general I'd say it's a good design. You use dependency injection and provide all required parameters via the constructor. You also use an abstraction (interface) for the service but... it's not very useful.


Why do you need an INotificationService? To be able to send notifications without knowing how they are going to be sent. You receive a notification-service and want to call the Notify method to send a notification. But do you want to repeatedly send the same notification? I don't think so. Thus the Notify method should probably require a message that you want to send.


I can imagine that such a service would have this API. You don't return bool but nothing if it worked and throw an exception if it didn't. If you don't throw exceptions then the user cannot know what went wrong if a service wasn't able to send a notification.

public interface INotificationService
{
    void Notify(string subject, string message);
}

Not all services may support a subject but a subject and a message are the two properties that actually every notification has.


Now the mail-service needs only the address (don't abbreviate names like adress):

public class MailService : INotificationService
{
    readonly string _address;

    public MailService(string address)
    {
        _address = address;       
    }

    public void Notify(string subject, string message)
    {
        // send notification
    }
}

The sms-service requires only the phone number and the sms-sender:

public class SmsService : INotificationService
{
    readonly string _phoneNumber;
    readonly ISmsSender sender;
    public SmsService(string phone, ISmsSender sender)
    {
        _PhoneNumber = phone;
        _sender = sender;
    }

    public void Notify(string subject, string message)
    {
        // send notification
    }
}

var smsNotification = new SmsService("12346789", "content" ,new SmsSender());

You shouldn't mix the vocabulary. If you create a SmsService then the instance is a smsService and not a smsNotification although in this case it is technically a notification because it can only send a single message over and over again. If this is the desired behavior then you shouldn't call it an INotificationService but an INotification. The names need to be clear otherwise the usage of your API is very confusing.


Ok, with the new design both services can still send messages but you can send a new message each time instead creating a new instance just to send a new message - so they are now real notification-services and not just notifications.

var smsService = new SmsService("12346789", new SmsSender());
smsService.Notify("Test1", "This is a message");
smsService.Notify("Test2", "This is another message");
\$\endgroup\$
  • \$\begingroup\$ i have another concern about this design. I'm consuming notification service in web api. Where could i inject the INotificationSevice if i do in constructor the data that are subject wont be available there. i.e, My api method is like this bool SendOtp(string phoneNumber) its http-post method. \$\endgroup\$ – Eldho Jan 12 '17 at 7:38
  • \$\begingroup\$ i have added detailed question here codereview.stackexchange.com/questions/152414/… . pls take a review @t3chbot \$\endgroup\$ – Eldho Jan 12 '17 at 8:02

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.