27

I had a little debate going on with a coworker. Simply put, is there a good reason to hide/encapsulate functions that are pure?

By "pure" I mean the wikipedia definition:

  • Always returns the same results from the same input. (For the sake of this discussion Foo Create(){ return new Foo(); } is considered impure if Foo does not have value semantics.)
  • Does not use mutable state (except local variables) or I/O.
  • Does not produce side effects.
Telastyn
  • 108,850
  • 29
  • 239
  • 365
  • 28
    There could be an argument made not to make such a function member of a class at all. – Pieter B Sep 26 '14 at 14:55
  • 6
    @PieterB - indeed, though not all languages support that, and some languages allow module internal even for free functions. – Telastyn Sep 26 '14 at 15:02
  • 4
    What was your opinion? I don't think the purity of the function is related to whether it belongs in the public API. – Andres F. Sep 26 '14 at 15:19
  • @andresF. - the debate was actually around whether an unbound validation rule should be made public. I made the argument that since it was a pure function, there was little harm. For this particular instance, it aided testability and was likely to be reused. This question is more about how broadly that argument could/should apply. – Telastyn Sep 26 '14 at 15:53
  • 2
    @Telastyn If it's reusable but isn't part of the current class' responsibility, it should probably be in a separate class/module/whatever-your-language-has. Then being part of the new class/module's responsibility, it would necessarily be made public. Since you mention testing, I'll note that you don't necessarily have to mock out the method when you do that. As an "implementation detail," mocking it during tests gives little benefit. – jpmc26 Sep 26 '14 at 17:27
  • @Telastyn (or anyone else): This is a dumb question, but what does "value semantics" mean? Does it just mean that a new instance of Foo created with an empty constructor is always the same, no matter when/how it gets instantiated? – Brian Snow Sep 26 '14 at 21:18
  • @BrianSnow - kind of. It means that `new Foo()` returns objects that are always equivalent (literally, `foo1 == foo2` is true) to each other. Compare that to reference semantics, where two different instantiations are not equal even though they contain the same data because they're different instances. – Telastyn Sep 26 '14 at 21:24
  • `Foo Create(){ return new Foo(); }` can never be pure since it can throw an exception if the allocation fails. – R.. GitHub STOP HELPING ICE Sep 27 '14 at 21:27
  • Please don't make every method public! I have to deal with code where even veteran developers write `public` from muscle memory even if the method is only used privately. It's very confusing because then I don't know if I can safely alter the method code for private use or not (without checking call hierarchy). If you can't determine the required visibility of a method, start with `private`. You can upgrade to default, `protected` and then `public` as requirements change. – ADTC Sep 28 '14 at 01:53

4 Answers4

79

A pure function could still be an implementation detail. Although the function may cause no harm (from the point of view of not breaking important invariants/contracts), by exposing it both the author and the users of that class/module/package lose. The author loses because now he can't remove it even if the implementation changes and the function is no longer useful to him. The users lose because they have to sift through and ignore extra functions that aren't relevant to using the API in order to understand it.

Doval
  • 15,347
  • 3
  • 43
  • 58
  • 34
    +1. Public members of your class is its API. It's not a question of "Can it hurt me if it's public?", but rather "Should it be part of the API?" – Ordous Sep 26 '14 at 16:18
  • 1
    Only thing I would add is that if you start seeing similar functions popping up in other locations, refactor it into another class so multiple classes can make use of it. Or if that's a concern to begin with, define it in a separate class to begin with. – jpmc26 Sep 26 '14 at 17:25
  • 1
    I would seal the public classes that are not meant to be extended as well. – Den Sep 30 '14 at 15:14
  • +1 for pointing out that hiding or showing a function (or class) are primarily questions of protecting and maintaining an API, and not related to the type of code it is. – Marco Sep 30 '14 at 20:09
43

The question is backwards.

You don't seek for a reason to make a function non-public. It is an incorrect mindset to start with (in my opinion). The reasoning should go the other way.

In other words - don't ask "why would I make it private?". Ask: "why would I make it public?"

When in doubt, don't expose it. It's kind of like Ockham's razor - don't multiply entitites beyond necessity.

EDIT: Addressing counterarguments brought up by @Telastyn in comments (to avoid extended discussion there):

I've heard that over time, and even espoused it for quite some time, but in my experience, things tend to be too private.

Yes, it's a pain sometimes if a class is open for inheritance, but you can't override some private methods (whose behaviour you'd like to alter).

But protected would suffice - and it's still non-public.

It leads to a lot of code duplication and overhead to get at "things that shouldn't be public" yet are accessed indirectly anyways.

If it becomes problematic, then simply make it public! There's the necessity I was talking about :)

My point is that you shouldn't do it just in case (YAGNI and all).

Note that it's always easier to make a private function public than pulling it back to privacy. The latter is likely to break existing code.

Konrad Morawski
  • 9,721
  • 4
  • 37
  • 58
  • I've heard that over time, and even espoused it for quite some time, but in my experience, things tend to be too private. It leads to a lot of code duplication and overhead to get at "things that shouldn't be public" yet are accessed indirectly anyways. – Telastyn Sep 26 '14 at 15:38
  • 4
    @Telastyn IMHO, that sounds like the application suffers from some design missteps; if you find yourself inclined to expose a method because you need to invoke it across discrete code paths, it's probably a sign that this method should be broken out into its own module that can be injected or included where appropriate, *especially* if it's a pure function. – leo Sep 26 '14 at 15:58
  • @leo - sorry, I don't follow. Consider a function like integer less than. This is a nice pure function that is exposed specifically because it can be then injected and included (or simply used) where appropriate. – Telastyn Sep 26 '14 at 16:23
  • 5
    @Telastyn In my opinion that's a symptom of the "everything is an object/class" philosophy combined with the fact that some OOP languages don't have composite data types other than classes. As soon as someone needs to pass two values at the same time they end up writing a class, and then every coworker and style checking software will tell you to make those fields private. – Doval Sep 26 '14 at 16:57
  • @Telastyn Right. What I mean is, if your 'integerLessThan' method started off as an encapsulated implementation detail of a public method, but you find that you need to invoke the private method in other places, that's probably a sign that the private method belongs in a different module/package/class so that it can be imported independently of the logic that invokes it. Simply publicizing the method as-is would mean the method is arbitrarily located in the first module where you found it useful. – leo Sep 26 '14 at 17:29
  • @leo - what? Why would integer less than not be right next to integer? – Telastyn Sep 26 '14 at 17:38
  • @Telastyn I think integer "less than" is not a good example, since it's a function that everyone agrees is part of the "public API" of integer numbers. In the general case, when you make something public it's harder to remove it later; private/public is more about "things that I can freely change with impunity" vs "things that are difficult to change". No-one would dream of removing integer "less than", but when you're coding anything else, you have to think about whether it's likely to change and/or disappear, and this is orthogonal to its "purity" (in the sense of the original question). – Andres F. Sep 26 '14 at 19:40
  • To clarify, I'm agreeing with this answer. – Andres F. Sep 26 '14 at 19:40
  • @Telastyn: the mindset is not wrong, what is wrong is **resisting** making a private method public. The default assumption should be that nobody else needs to know about it, once that assumption has been disproven, there should be very little reistance to changing what is exposed, although you may want to go with the least exposure that makes sense at that time. – jmoreno Sep 28 '14 at 16:20
6

I don't think the decision to hide/encapsulate a function should depend on its purity. Just because a function is pure doesn't mean that externals need to know about it. Interestingly enough though if the function is pure and meant to be public maybe it doesn't even need to be an instance member of the interface at all maybe it is better suited as a static. But again all of this depends on the intent of the contract and in this case the logical grouping of functionality, not the purity of the function.

pllee
  • 1,212
  • 1
  • 9
  • 13
5

Classes should adhere to the Single Responsibility Principle. While a class may need to call on other functionality to achieve its goals, it should only expose functions that are part of its single responsibility.

Here is just one example of a case where visibility could cause a problem.

Consider a class that frobnicates widgets. Maybe as part of its frobnication code it needs some utility function that parses a string: perhaps it needs to transform the widget name in a way that standard string functions do not support.

Since this is a pure function (string comes in, transform it somehow, return a new string), it could be public or private without consequence. Or could it?

If you make it public, now your class has two responsibilities: frobnicating widgets, and transforming strings. This violates SRP, and can cause problems if other classes come to rely on the function. Since this is something you think is only used internal to the class, perhaps you change its interface or visibility. Now classes in other parts of the system are broken.

By keeping the function private, nobody ever has the opportunity to rely on code that is not part of the class's single responsibility.

  • 2
    I would argue that it should only _contain_ functions that are part of its single responsibility, not just expose. – Telastyn Sep 26 '14 at 15:37
  • 1
    I think there is a gray area. Do you really need a separate `StringTransformer` class to encapsulate two or three lines of code that are only used in one place? I agree that once code is used in multiple places it is best to separate it into a new class with one responsibility, but there is a tradeoff. –  Sep 26 '14 at 15:40
  • Certainly. Guidelines are guidelines, not rules. – Telastyn Sep 26 '14 at 15:45
  • @snowman in non java you just make a library of functions. – Pieter B Sep 26 '14 at 16:50
  • @PieterB it is not about Java v. anything else. To make it truly OO you would need a factory, some abstract classes, etc. –  Sep 26 '14 at 17:10
  • @Snowman I think you have a strange definition of "truly OO". If `1 + 2` is an acceptable shorthand for `1.plus(2)` then why can't `f = function(...){...}; f(x);` be acceptable shorthand for `f = new Function(...); f.call(x);` ? A "library of functions" is a collection of objects, if functions are first-class objects. – Warbo Sep 26 '14 at 17:53
  • @Warbo sarcasm does not come through well here, sorry. I was alluding to the fact that many OO solutions are more complex than the problems they claim to solve. –  Sep 26 '14 at 18:57