0

This is the first thought I got while reading Interface Implementation (Interface Segregation Principle)

Thought

To introduce a new interface that would represents method parameters instead of passing individual parameter values. As shown below:

interface IServiceProviderInput
{
    string Username { get; } 
    string Password { get; }
    string AgentId { get; } // XYZServiceProvider needs this.
    // Similarly add props here to represent new parameters 
    // required by future service provider implementations.
}

interface IServiceProvider
{
    bool Authenticate(IServiceProviderInput parameters);
}

class ABCServiceProvider : IServiceProvider 
{
    public bool Authenticate(IServiceProviderInput parameters) 
    {
        return true;
    }
}

class EFGServiceProvider : IServiceProvider 
{
    public bool Authenticate(IServiceProviderInput parameters) 
    { 
        return true;
    }
}

class XYZServiceProvider : IServiceProvider
{
    public bool Authenticate(IServiceProviderInput parameters)
    {
        return true;
    }
}

Question

Would this make sense or what are the flaws in this? Any thoughts?

Edit

Another thought to add more specific interface for XYZ provider:

interface IServiceProviderInput
{
    string Username { get; } 
    string Password { get; }
}

interface IXYZServiceProviderInput : IServiceProviderInput
{
    string AgentId { get; }
}

class XYZServiceProvider : IServiceProvider
{
    public bool Authenticate(IXYZServiceProviderInput parameters)
    {
        return true;
    }
}

It is possible that both the thoughts are incorrect or have flaws, I am not sure, hence the question.

Community
  • 1
  • 1
Nikhil Vartak
  • 5,002
  • 3
  • 26
  • 32
  • Why do the input properties have setters? That smells odd to me. Would you expect a service provider to be able to change the property values of the input? – recursive Jun 17 '16 at 18:01
  • @recursive Services won't set them. How would calling code set the parameter values otherwise? – Nikhil Vartak Jun 17 '16 at 18:49
  • 1
    Calling code doesn't need to access them exclusively via this interface. The purpose of an interface is to group together related operations by responsibility. If that responsibility is to act as an input, then a setter isn't needed on the interface. But that doesn't mean a setter can't exist on an implementation or a more derived interface. To use another example, you can't `.Add()` an item to an `IEnumerable`, even though you can on a `List`. – recursive Jun 17 '16 at 20:50
  • *a setter isn't needed on the interface. But that doesn't mean a setter can't exist on an implementation or a more derived interface* - Completely forgot basics. Thanks for the revision :) +1. Updated code. – Nikhil Vartak Jun 17 '16 at 21:18

2 Answers2

2

You certainly could do this but unless ALL the methods accepted and needed ALL the paramaters defined by the interface its a terrible idea. Never pass more information to a method then it NEEDS otherwise you have no clue what's required to work and what isn't from the interface.

Paul Swetz
  • 2,234
  • 1
  • 11
  • 28
  • So to provide only required parameters I mentioned thought #2 where I created `IXYZServiceProviderInput` but yes it will be even worse or impractical thing to do so for multiple methods. So do we conclude that this is actually bad idea and should be strictly avoided? Or wait for more thoughts from others? – Nikhil Vartak Jun 17 '16 at 18:15
  • Chances are you will eventually have interfaces for every methods parameters. I'm sure someone will argue "Makes for great unit testing" but without specific reasons to be doing I would not recommend it. Now if we are really following OOP where say all these functions take an IPerson and conceptually work on IPerson data then its much more acceptable but even then you want to avoid having a path to construct a Person object in a state where methods could fail unexpectedly because the user forgot to set some field that there is no obvious way for them to know was required. – Paul Swetz Jun 17 '16 at 18:22
0

My only question with your implementation is why you are using the same authenticate method with the XYZServiceProvider but not implementing the IServiceProvider interface that you defined?

The problem with this is when you have clients that are calling the IServiceProvder for authentication they will not be able to use the XYZServiceProvider but will only able to use the other two. If they want to use the XYZServiceProvider they will need to specify it by name (tightly coupled).

If you were going to make changes to your program, and switch from XYZServiceProvider to EFGServiceProvider to authenticate with, then the individual clients would also need to change their code so that they could use the new provider. This is especially troublesome when trying to create unit tests for your services because you will have to individually create test fixtures for XYZServiceProvider.

If you were going to have an alternate type of service for the XYZServiceProvider I would recommend creating another interface like the IServiceProvider that is used for the other services.

ghg565
  • 422
  • 6
  • 15
  • Ahh, You changed it before I posted my answer. That is better – ghg565 Jun 17 '16 at 18:08
  • I changed it post your answer otherwise I would not have reaslized what I missed. You thought were valid for previous code version. Thanks for pointing that out. What do u think about the updated and final code now? – Nikhil Vartak Jun 17 '16 at 18:11
  • I still think that the the XYZServiceProvider should implement the IServiceProvider interface. The parameter is still of the type IServiceParameterInput so this would allow you to authenticate to `IServiceProvider.Authenticate(IServiceProviderInput parameters)` and then keep the specifics inside the implementation. – ghg565 Jun 17 '16 at 18:13
  • I don't get you. How would you pass `agentId` parameter then? – Nikhil Vartak Jun 17 '16 at 18:21
  • Because your IXYZServiceProviderInput inherits from IServiceProviderInput it is still of the type IServiceProviderInput. – ghg565 Jun 17 '16 at 18:25
  • Looks good to me now. That is a nice implementation that allows for Inversion of control. – ghg565 Jun 17 '16 at 18:31