Background:
We have a class that both listens on a socket and sets values on itself based on what it reads off the socket. I believe that adheres to SRP.
To adhere to ISP we created one interface for socket stuff (eg. StartListening, StopListening) and another interface which contains read only fields for the values read off the socket and modified our class to implement both.
The idea was that we pass ISocketStuff (not its real name) to code that needs to start/stop listening and we pass ISocketValues to places that need to know the values.
The Question:
I was reviewing a co-worker’s code and he had something like:
public interface ISocketStuffAndValues : ISocketStuff, ISocketValues {};
And he was passing it to a constructor.
This felt wrong, but I couldn’t think of any reason not to do this. It was to work around a thorny design issue, he could have just passed in ISocketValues, but he wanted to do it that and I couldn’t tell him why it was a poor choice.
So, is it a poor choice? What reasons could I have given for just passing in ISocketValues and not creating ISocketStuffAndValues?
3
Your coworker was trying to reduce another code smell; having many and/or redundant parameters in method calls. If you adhere to ISP, SRP and DIP by allowing different implementations of the two interfaces to be passed into a method that needs an implementation of each, but you have one class that implements both interfaces, then when you call this method you’ll be passing the same instance twice, once as an ISocketStuff and once as an ISocketValues. Your coworker thought that smelled bad (and he’s right), and refactored to allow a single passed reference.
It’s always a good idea, instead of blindly following the SOLID rules, to remind yourself why these rules exist; what’s their purpose? in the case of ISP, the purpose of the rule is to prevent having to make changes to a consumer of an interface that does not consume the particular method of that interface that is being changed. However, if all usages need all methods, that’s clear evidence that the methods of the interface are cohesive, at least as far as your system is concerned, and they don’t need to be split up.
The right answer to your question thus depends on the answer to this question: will all consumers of an implementation of ISocketStuff be expected to need ISocketValues as well?
If so, then your assertion that these need to be separate interfaces is unfounded. If every implementation of one implements the other, and every consumer of one consumes both, then if the interfaces change, all implementations and usages of both interfaces will change, and so you gain nothing in segregating them. The members of both interfaces would be highly cohesive in a single interface. The code smell of re-joining both interfaces into a child interface used by implementations and consumers is your red flag that you have over-segregated given the needs of your system, and ISocketStuff should simply expose the members of ISocketValues.
If not, then you have other problems. It’s not bad, per se, for an object to implement multiple interfaces; that’s part of why interfaces exist and why implementing many of them is allowed by language specs in the first place. However, in this case, an object with the single responsibility of socket listening and an object with the single responsibility of storing status/data values are two objects with two responsibilities and should not have a single, shared implementation. Instead, consider making the implementation of ISocketValues a “thin” data class (a DTO), that is produced by a method exposed by ISocketStuff.
ISocketValues could be cheap and disposable, and you get one whenever you need an update and then throw it away, or alternately instances of ISocketValues could be longer-lived and receive “live” updates from ISocketStuff. Which is better depends on how you prefer to use both interfaces, and which of the interfaces you want other objects to have to know about. If most or all consumers need to control the listener based on updates, then they need ISocketStuff; make ISocketValues’ implementation a DTO, expose a factory method to generate them, and pass ISocketStuff around to consumers, who can generate ISocketValues instances as needed. If all your consumers need is data/status info, then ISocketStuff is too much power; the owner of ISocketStuff that is injecting these dependencies into callers should generate and pass “live-updating” ISocketValues implementations to consumers, who will no longer have to know ISocketStuff even exists.
4