difference between expected inputs and implementation details

Question

During a code review, I suggested to my coworker that he had questionable parameters for one of his methods. In particular, the parameters revealed too much about the method's implementation details. My coworker argued that a method's inputs must reveal the method's implementation details, so, where does one draw the line? I could kinda see what he was saying, but I struggled to articulate the distinction between "inputs" vs "implementation details".

Contrived examples (he would argue all of these are OK):

• int Math.abs(int someNumber); // ok to me
• int Math.abs(Integer someNumber); // slightly interesting to me
• int Math.abs(int someNumber, SignedHelper signedHelper); // SignedHelper is a class that tells you whether numbers are signed or unsigned. WTF to me.

Things I've said that are unsatisfactory to him:

• You would expect Math.abs() to take a primitive type. Why should it be a capital-i Integer?
• The presence of SignedHelper reveals too much about how Math.abs() works.
• You would expect Math.abs() to figure out what a signed number is - the user shouldn't need to care. That's an implementation detail.
• The presence of SignedHelper implies that it's possible to pass an implementation of SignedHelper that behaves differently than what you'd expect (which could be a possible feature, I guess, but in our case wasn't an ask or expectation at all). This is puzzling and raises questions about the motives of this method.
• If we pretend that Math.abs() actually DOES use SignedHelper in its implementation, it's better to test SignedHelper separately than to expose it in the parameters to enable injection of a mocked SignedHelper. (this was in response to one of his concerns, where he wanted to test only the code that used SignedHelper, rather than testing any of SignedHelper along with the code in question)

So, how would you explain the difference between inputs that make sense vs inputs that don't make sense? I sense he is being argumentative but at the same time I'd like to be able to articulate something that is just obvious to me, because it's quite possible I actually don't have a good understanding.

Answer 1

I'd say 'Math.abs(int someNumber, SignedHelper signedHelper)' is violating SRP(single responsibility principal). That is why it looks wrong.

You're passing an argument to the method and at the same time supplying a dependency the class needs.

Construction is a separate responsibility to ABS and the method is conflating the two. If the class needs SignedHelper as a dependency it should be injected into the constructor.

Answer 2

The signature of a method is the interface to the callers of the method.

In your case:

int Math.abs(int someNumber);

tells you that Math.abs() takes an integer number and returns another integer number. An important principle is that an interface should not change if the implementation changes.

In your context, if all the relevant implementations require a SignedHelper object, then it may be OK to pass it as a parameter. In this way you have a generic method Math.abs() that works with any implementation of SignedHelper.

On the other hand, if you can also have implementations of Math.abs() that do not use a SignedHelper at all, then you are correct that there should be no SignedHelper parameter: this is an implementation detail that should stay hidden inside the implementation and not leak to the interface.

What is the problem in the second scenario? Imagine you change the implementation to one that does not use a SignedHelper. Then

• either you need to change the interface and change all calls to the method throughout your code,
• or you keep the interface and ignore the argument in the implementation; the callers keep sending a dummy object just because it happens to be in the interface.

In the first case you are changing an interface: sometimes this is inevitable but changing an interface is a non-local change so it should be avoided if possible.

In the second case your interface does not describe the method properly. An API that requires dummy parameters is not well designed.

Answer 3

These two signatures

Math.abs(int someNumber);

and

Math.abs(int someNumber, SignedHelper signedHelper);

Are somewhat comparable to the two versions of toString from the Integer class:

public static String toString(int i)

and

public static String toString(int i, int radix)

---

Both methods compute a result from an int and can do so without anything else given as a parameter. "Take the int and do your thing."

But maybe you want to get a string representation with a different radix. And maybe the internal implementation is capable of being variable in that regard, then why not expose that as another parameter?

That's exactly what happens in both cases. You have the option to modify how the internal implementation works.

Take the constructor of BufferedReader as another example:

public BufferedReader(Reader in, int sz)

WTF to me.

If you are not used to having the choice to specify the size of a "buffer" when working with a Reader, the additional parameter might come across as revealing information of the internal implementation - which is perfectly fine, because that's the whole point.

You have the option to pick a specific implementation if you need to and in that situation you definetly want those additional parameters. If you pick BufferedReader on purpose, you very likely want to specify the buffer size.

If you pick Math.abs(int someNumber, SignedHelper signedHelper) on purpose, you very likely want to specify what SignedHelper should be used.

The presence of SignedHelper implies that it's possible to pass an implementation of SignedHelper that behaves differently than what you'd expect (which could be a possible feature, I guess, but in our case wasn't an ask or expectation at all). This is puzzling and raises questions about the motives of this method.

Whether its puzzling or not seems to be somewhat subjective. I often find overloads for methods in APIs that I cannot think of a use for. But somebody else might as well find them very useful.

Is there any harm in simply providing both versions of the method? Despite not being a requirement, is the version accepting the SignedHelper used anywhere in the code?