3
\$\begingroup\$

I am working on some sampling to collect data for my research on SOLID Principles. One of my sample consists proceeding code snippet:

public abstract class Notify
{
    public abstract void NotifyClient();
}

public class OnPremisesClient : Notify
{
    public override void NotifyClient()
    {
        Console.WriteLine("You're getting these notifications because you opted....");
    }
}

public class CloudClient : Notify
{
    public override void NotifyClient()
    {
        Console.WriteLine("You're getting these notifications because you opted....");

        if (IsOnPremisesToo)
            NotifyClientAsOnPremisesClient();
    }

    public void NotifyClientAsOnPremisesClient()
    {
        Console.WriteLine("Awesome! You are also using On premises services...");
    }

    public bool IsOnPremisesToo { get; set; }
}

Calling class is:

public  class Program
    {
         public static void Main(string[] args)
          {
              var premisesClient = new OnPremisesClient();
              var cloudClient = new CloudClient();

              ProcessNotifications(new List<Notify> { premisesClient, cloudClient });
          }

          private static void ProcessNotifications(List<Notify> list)
          {
               HandleItems(list);
          }

          static void HandleItems(IEnumerable<Notify> notifications)
          {
              foreach (var notification in notifications)
              {
                 if (notification is CloudClient)
                 {
                     var cloudClient = notification as CloudClient;
                     cloudClient.IsOnPremisesToo = true;
                 }
                 notification.NotifyClient();
              }
          }
    }

In the preceding code snippet, I am trying to notify the client as per the type of Notify client could be a OnPremisesClient or CloudClient.

This code-snippet looks neat and clean, but I would like to discuss which SOLID principle it violates. After going through few SOLID resources, I thought it violates SRP as it uses if. In the future, if there will be new client like GalaxyClient, then this code need a new condition. There might be more violations.

Are SOLID principles really violate in the give code-snippet or just I am thinking it violates SOLID? I would appreciate it if someone tells the which principles are violating with reasoning. What would be the new code or what are changes should made to this code so, it'd follow SOLID Principles?

\$\endgroup\$
  • \$\begingroup\$ throw new NotImplementedException(); - broken code is off-topic. \$\endgroup\$ – t3chb0t Aug 13 '16 at 22:00
  • 1
    \$\begingroup\$ @t3chb0t: The code isn't "broken"; the line throw new NotImplementedException() is obvious boilerplate, in the same way as one might write foo(); or // omitted for brevity or /* ... */. \$\endgroup\$ – Quuxplusone Aug 13 '16 at 22:22
  • \$\begingroup\$ replaced throw new NotImplementedException(); with correct function calling. \$\endgroup\$ – Shuby Aug 13 '16 at 22:29
  • \$\begingroup\$ @Quux And that's off topic too. \$\endgroup\$ – Nic Hartley Aug 13 '16 at 22:47
  • \$\begingroup\$ Yes, code sample is violating SOLID principles. \$\endgroup\$ – Gaurav Aroraa Aug 14 '16 at 8:03
3
\$\begingroup\$

The code in example is violating following principles:

  1. Single Responsibility Principle(SRP): The responsibility of Notifying is spread thin across Clients and the classes are not clear with their intentions and it is also not reflecting through the behaviours. Now, it is not clear whether the client is also supposed to do other jobs.
  2. Liscov Substitute Principle(LSP): 'Is-A' Relationship should be replaced with 'Is-Substitue-for' relationship. notification is CloudClient is violating LSP.
  3. Dependency Inversion Principle(DIP): is violated since program class is dependent on client implementations and Notify class.

IMO, code can be restructured as follows. Please note, it can be also improved with better DI Implementation. By judging the code listing, I am not clear what is intended output of this program. Whether it is sending two different notifications or three notifications. Still,trying to provide the answer as per my understanding and assumptions below. Please let me know in comments the intent of the program so that I can modify the program as per the intent.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace CodeSmellQuestion
{
    public class Notification
    {
        private readonly List<INotify> _providerList;
        public Notification(List<INotify> providerList)
        {
            _providerList = providerList;
        }

        public void SendAll()
        {
            foreach (var notificationProvider in _providerList)
            {
                notificationProvider.Notify();
            }
        }
    }

    public interface INotify
    {
        void Notify();
    }

    public class OnPremiseNotifier : INotify
    {
        public void Notify()
        {
            Console.WriteLine("You're getting these notifications because you opted for OnPremise Notifications....");
        }

    }

    public class CloudNotifier : INotify
    {
        public void Notify()
        {
            Console.WriteLine("You're getting these notifications because you opted for Cloud Notifications....");
         }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I am not completely agreed with your code-snippet. Think what happened when we have new notifier say GalaxyNotifer as per your code we have to pass new constructor parameter - will that ok and what about when we've fourth notifier. \$\endgroup\$ – Shuby Aug 13 '16 at 23:27
  • \$\begingroup\$ @Shuby Please check the updated code. This will take care of new 'GalaxyNotifier' requirement and if my answer help, please dont forget to mark it as answer. \$\endgroup\$ – ATechieThought Aug 13 '16 at 23:38
  • \$\begingroup\$ This would be a better answer if it gave examples of when the code violated each principle and what your solution was. As is, it would take a significant amount of work for most readers to see what you changed and work back to why. \$\endgroup\$ – mdfst13 Aug 14 '16 at 0:22
  • \$\begingroup\$ @ATechieThought - in method Notify of class CloudNotifier just after Console, you've written .Notify(); is it typo? \$\endgroup\$ – Gaurav Aroraa Aug 14 '16 at 10:02
  • \$\begingroup\$ Added a fiddle: dotnetfiddle.net/vXg3Lq \$\endgroup\$ – Gaurav Aroraa Aug 14 '16 at 15:49
2
\$\begingroup\$

DIP (Dependency Inversion Principle)

The OnPremisesClient class violates the DIP (Dependency Inversion Principle) and the IoC (Inversion of Control) because it uses the Console internally directly. You should pass an interface or an abstract class that exposes a Write(Line) method.

(The same in other classes that use the Console)

HandleItems

I'm not sure which principle it violates (SRP or LSP) but it's definitely wrong to implement execptional handling for the CloudClient there. You need to rethink the base class. Why does this one need this property any why do you need to set before sending a notification? Are there more types that might require it? If so then maybe it's a good idea to move it to the base class but be careful about the ISP to not force it for clients that might not need it.

If only some clients require the IsOnpremissesToo then maybe they should implement some interface like IPremmissable to decouple it from the conrete clients.


Example

The other reviewers already mentioned that it is not very clear what you are going to do so this won't be an exact solution but just a proof of concept and a general idea. There are countless possibilities for the design but for now I picked the following:

You start with a INotifier interface:

public interface INotifier
{
    void Send(string message);
}

and you use it to implement concrete notifiers like:

public class ConsoleNotifier : INotifier
{
    public void Send(string message)
    {
        Console.WriteLine(message);
    }
}

public class DebugNotifier : INotifier
{
    public void Send(string message)
    {
        Debug.WriteLine(message);
    }
}

Next you modify the base client class to accept a list of notifiers and that requries the derived classes to implement the CreateMessages method (this is one of the Factory Patterns where the derived class provides something conrete for the base class - here just strings):

public abstract class Client
{
    private readonly IEnumerable<INotifier> _notifiers;

    protected Client(IEnumerable<INotifier> notifiers)
    {
        _notifiers = notifiers;
    }

    protected abstract IEnumerable<string> CreateMessages();

    public void NotifyClient()
    {
        var messages = CreateMessages();
        foreach (var notifier in _notifiers)
        {
            foreach (var message in messages)
            {
                notifier.Send(message);
            }
        }
    }
}

now each client requires a list of notifiers and it creates the appropriate messages:

public class OnPremisesClient : Client
{
    public OnPremisesClient(IEnumerable<INotifier> notifiers) : base(notifiers) { }

    protected override IEnumerable<string> CreateMessages()
    {
        yield return "You're getting these notifications because you opted....";
        yield return "Awesome! You are also using On premises services...";
    }
}

public class CloudClient : Client
{
    public CloudClient(IEnumerable<INotifier> notifiers) : base(notifiers) { }

    protected override IEnumerable<string> CreateMessages()
    {
        yield return "You're getting these notifications because you opted....";
    }
}

Finally you can notify the clients:

public static void Main(string[] args)
{
    var clients = new Client[]
    {
        new OnPremisesClient(new[] { new ConsoleNotifier() }),
        new CloudClient(new INotifier[] { new ConsoleNotifier(), new DebugNotifier() }),
    };
    NotifyClients(clients);
}

static void NotifyClients(IEnumerable<Client> clients)
{
    foreach (var client in clients)
    {
        client.NotifyClient();
    }
}
\$\endgroup\$
  • \$\begingroup\$ @Shuby I've added an example. \$\endgroup\$ – t3chb0t Aug 14 '16 at 8:09
0
\$\begingroup\$

Just to lay emphasis on what's been said by other reviewers ;

  • Liskov Substitution: I should be able to replace var with Notify since Notify is the base class and it's behaviour should be to print out a message
Notify premisesClient = new OnPremisesClient();
Notify cloudClient = new CloudClient();

Notice, the second line violates this principle as calling this has changed the behaviour of Notify .

Refactoring your code will involve taking that extra line in the Notify of the CloudClient

public void NotifyClientAsOnPremisesClient()
{
      if (IsOnPremisesToo){          
        Console.WriteLine("Awesome! You are also using On premises services...");
      }
}

P.S I wouldn't be surprised if you get this error "CS1061: Type Notify does not contain a definition for NotifyClientAsOnPremisesClient() and no extension of the type Notify could be found.
There are different ways to solve this:

  1. Adding the NotifyClientAsOnPremisesClient to your abstract class.

  2. Creating a separate class that deals with CloudClient that are also OnPremisesClient. Going by the approach 1, you should end up with

     public abstract class Notify {
       public virtual string PremisesMessage {
         get;
         set;
       }
       public abstract string NotifyClient {
         get;
         set;
       }
       public abstract string NotifyClientAsOnPremisesClient(bool IsOnPremises);

     }

The PremisesMessage gets and sets the PremisesMessage. This is not a good name for this but for the sake of explanation this should. I will also explain the other pieces.

  • Personally Speaking, I like to restrict my writeLine statements to the Main method. Hence I used get..set for the NotifyClient definition.
public class OnPremisesClient: Notify {
  //   public override string NotifyClient()
  //   {
  //      Console.WriteLine("You're getting these notifications because you opted....");
  //  }
  public OnPremisesClient(string Message) {
    NotifyClient = Message;

  }
  private string _notifyClient = "";
  public override string NotifyClient {
    get {

      return _notifyClient;

    }
    set {
      _notifyClient = value;
    }

  }
  public override string NotifyClientAsOnPremisesClient(bool IsOnPremises) {

    if (IsOnPremises) {
      PremisesMessage = "Awesome! You are also using On premises services...";
      return PremisesMessage;
    }
    return "";
  }
  public override string PremisesMessage { get; set; }
}

The class where you've your Main method should be looking like this

public class Program {
  public static void Main(string[] args) {
    Notify premisesClient = new OnPremisesClient("You're getting these notifications because you opted....");
    Notify cloudClient = new CloudClient("You're getting these notifications because you opted....");
    premisesClient.NotifyClientAsOnPremisesClient(false);
    cloudClient.NotifyClientAsOnPremisesClient(true);
    ProcessNotifications(new List < Notify > {
      premisesClient, cloudClient
    });
  }

  private static void ProcessNotifications(List < Notify > list) {
    HandleItems(list);
  }

  static void HandleItems(IEnumerable < Notify > notifications) {
    /* foreach (var notification in notifications)
     {
        if (notification is CloudClient)
        {
            var cloudClient = notification as CloudClient;
            cloudClient.IsOnPremisesToo = true;
        }
        notification.NotifyClient();
     }*/
    foreach(var notification in notifications) {
      Console.WriteLine(notification.NotifyClient);
      Console.WriteLine(notification.PremisesMessage);
    }

  }
}

Note, The more robust way will be to decouple NotifyClientAsOnPremisesClient just like @t3chb0t suggested. P.S you could also improve on your naming convention as most of the names you've used do not justify their functionality. I hope this helps.

\$\endgroup\$

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.