2

This question is regarding Interface Segregation Principle Best-Practices. I use abstract examples below, but the question arises from actual code I've seen that performs what I can only call "Implementation Coupling".

If I were to break my concrete MilkMan class into his respective roles, I would see that MilkMan has three roles. He plays the role of a Person (walks and chews bubblegum), He plays the role of a Deliverer (Inventory & Accounting functions?), and He hits on my wife when I'm not at home.

Then when I am writing/reviewing my Accounting logic (or Unit Testing it), I see that some methods in my accounting class(es) can take a MilkMan parameter. When I see this, I obviously have something wrong. The only thing my accounting class (Or Unit-Tests of such) cares about is his Deliverer role.

So what I do is identify the roles of my MilkMan, and segregate some interfaces:

  • IPerson
  • IDeliverer
  • ICovetThyW

Now when my accounting class (or its Unit-Tests) needs to mock or pass around Deliverer functionality, the relationships are clear. I can now have my MailMan, UpsMan, and PoolBoy talk to my Accounting logic by clearly passing around IDeliverer.

So now Unit Testing is much easier to mock, and relationships become more clear in my code. Everything is great.

What if one method in one implementation is making calls to another method in the same class, but which is part of another implementation? Take the following code into consideration:

namespace HomeAccounting.KeepAnEyeOnPayments.Sample
{
    class Program
    {
        static void Main(string[] args)
        {
            IDeliverer jose = new MilkMan();
            double paymentAmount = 69;
            if (jose.AcceptPaymentFromCustomer(paymentAmount))
            {
                System.Console.WriteLine("He got " + paymentAmount.ToString());
            }
        }
    }

    public class MilkMan : IPerson, IDeliverer, ICovetThyW
    {
        public bool CanWalk { get; set; }
        public bool CanChewBubblegum { get; set; }

        public bool AcceptPaymentFromCustomer(double amountDue)
        {
            if (GetPaymentFromCustomer(amountDue) == amountDue)
            {
                return true;
            }
            else
            {
                if (AlternativePaymentMethod(amountDue) == amountDue)
                {
                    return true;
                }
            }
            return false;
        }

        private double GetPaymentFromCustomer(double amountDue)
        {
            return 0; //Not willing and/or able to give money.
        }

        public double AlternativePaymentMethod(double amountDue)
        {
            return amountDue;
        }

    }

    public interface IPerson
    {
        bool CanWalk { get; set; }
        bool CanChewBubblegum { get; set; }
    }

    public interface IDeliverer
    {
        bool AcceptPaymentFromCustomer(double amountDue);
    }

    public interface ICovetThyW
    {
        double AlternativePaymentMethod(double amountDue);
    }
}

The question is: Does anybody see anything wrong with this? But in all seriousness: I'd like to know if there is a term or discussion or best-practice listed out there that says you shouldn't be coupling your implementations together.

I want to point out that explicit interface implementation (Which I push for) makes this not even possible to do. But I've honestly never seen that in a professional environment in 15 years.

1

I actually think this is one of the major advantages to being able to have a class, that implements multiple interfaces, while having one "implementation". I think the problem is with your example, than with the idea. I remember I did similar solution some time back.

Basically, I created a behavior, that did some work and it required some work before and after it on same files in single folder. To do this, I created two interfaces : IBeforeWork and IAfterWork. The, there was a class, that looked like this:

public class FileChecker : IBeforeWork, IAfterWork
{
    private string directory;
    private string checksum;

    public FileChecker(string dir)
    {
        this.directory = dir;
    }

    void IBeforeWork.DoWork()
    {
        // calculate checksum from files in directory
    }

    void IAfterWork.DoWork()
    {
        // check checksum on files in directory
    }
}

Then, I plugged one instance of this class in both places and had perfectly decoupled checksum checking for files in directory.

0

Does anybody see anything wrong with this? ...

I think I see an over-use of interfaces because ...

in all seriousness

When I read this: What if one method in one implementation is making calls to another method in the same class, but which is part of another implementation? the code smell is clear.

I see a natural inheritance situation being corrupted by mis-application of the interface segregation principle. You're disconnecting what an object is from what it does via ISP. "Delivery" behavior is a natural extension of what the object is. An interface is used to give common behavior to unrelated classes. This is not your situation.

Redesign to inherit delivery behavior

Seems to me you have one customer delivery/payment system. One process that can be embodied in a base class. This better expresses, reflects, and simulates your real world. With multiple implementations of the same thing you are asking for maintenance headaches.

I see that some methods in my accounting class(es) can take a MilkMan parameter. When I see this, I obviously have something wrong.

... because of the design you're trying to force. If instead MilkMan inherits DeliveryPerson inherits Person then that Milkman parameter doesn't need casting and he just does his delivery thing. And likewise for any DeliveryPerson subclass.

Separate Accounting functionality

The milkman delivers the milk and then uses the payment process to collect. Consider making CustomerPayment a separate class - this is what you are really after I suspect. First, as its own class we are more explicitly modeling this particular accounting process. Second, if the DeliveryPerson has some kind of account, or ledger, or sales book or... then the CustomerPayment process (class) merely uses that.

Or maybe each customer has a separate account; but still the CustomerPayment class uses that. Maybe both - it updates the delivery person's sales and the customer account.

I'm imagining that the DeliveryPerson has a reference to the CustomerPayment class (i.e. the milkman is holding a hand-held device to process the customers order). And the milkman presses buttons on the hand-held (calls CustomerPayment methods) to process the order.

Other Points

Milkman violates the single responsibility principle. He has no business having account processing implementation.

That you're duplicating this accounting behavior is a code smell, and it stinks far more than interface separation issues.

The undesirable coupling you should worry about is with the user interface and database. This is coupling that prevents unit testing.

Good separation of class responsibilities, primarily, should allow your unit test setup to easily build any composite objects. Then mocking becomes less of an issue.

Interface-segregation principle does not say "favor lots of itty-bitty interfaces over inheritance." I'll be the first to say that unit testing helps refine my design but for crying out loud, dear reader - design first.

Other thoughts on the over-use of interfaces

  • This is good information. I apologize for a common mistake that I've made where an abstract code example causes confusion. I agree with almost all of your points, but only a very small part of it related to the question, which is; whether a class implementing more than one interface should have implemented methods of those interfaces calling other implementation's methods. – Suamere Oct 19 '14 at 21:48
  • Perhaps I am misunderstanding, but the question itself suggests the problem(s) I discussed. Further the chain of interface calls suggests potential problems discussed in the link at the very bottom of the post. – radarbob Oct 20 '14 at 0:01

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.