4

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?

  • 6
    I'm not 100% positive without seeing more of your design; it sounds like your class that both listens on a socket and sets values may violate SRP. – Aaron Kurtzhals May 30 '13 at 14:46
  • I agree with @AaronKurtzhals here, break those implementations up into 2 separate classes and then see how the design lays out, I think you'll find then that the correct approach is clearer and the issue you're raising here will become moot. – Jimmy Hoffa May 30 '13 at 15:07
  • @AaronKurtzhals, Does a class that listens to a socket GPS co-ordinates, for example, break SRP if it has a Latitude property? From Wikipedia "Martin defines a responsibility as a reason to change, and concludes that a class or module should have one, and only one, reason to change." The only reason I can think of for such a class to change would be that the format of the message changes. – Robert Gowland May 30 '13 at 15:54
6

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.

  • Thanks, KeithS, that helps frame the discussion. The answer to the question " will all consumers of an implementation of ISocketStuff be expected to need ISocketValues as well?" is "no". Only the main method of the app and a watch-dog method need to start/stop listening (and neither needs the values from the socket). So, as a follow up question... Ultimately, the class that listens to the socket has to make its data available somehow. If there are classes that always need the latest values, how else could I have designed this? – Robert Gowland May 30 '13 at 17:12
  • As I mentioned, if most consumers of data don't need to control the socket, then what they can be given is a separate implementation of ISocketValues that is generated by an ISocketStuff implementation. The ISocketStuff implementation can remember that this instance exists, and when ISocketStuff decides that data must change, it updates any and all instances of ISocketValues. Alternately, ISocketValues could remember which ISocketStuff instance it was generated by, and expose a "Refresh()" method to accomplish much the same goal at the consumer's demand. – KeithS May 30 '13 at 18:53
  • Either way, ISocketStuff's implementation must know about ISocketValues, and/or vice-versa, but none of the consumers of either interface have to care that the other one exists, except for (ideally) one usage which uses an ISocketStuff implementation to generate ISocketValues instances as needed to inject into consumers of ISocketValues. – KeithS May 30 '13 at 18:56
  • @KeithS they don't need to know each other, if you have one class which knows them both, and reads the data updating the values, now neither socket or values need know anything about each other. This tends to be my preferred approach to retrieving data from a resource and putting it into a model, a resource class, a dto, and a class for filling the dto from the resource. – Jimmy Hoffa Jun 3 '13 at 5:23

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.