27

Is it a code smell to call public method in private method of the same object instance?

Tulains Córdova
  • 39,201
  • 12
  • 97
  • 154
Eimantas
  • 1,026
  • 1
  • 8
  • 17
  • AAMOI do you have a specific example? – ocodo Jan 12 '11 at 10:58
  • No, not currently. Just remembered a case which i discussed with my colleagues. Wanted to get some opinions here as well. – Eimantas Jan 12 '11 at 11:15
  • 5
    Usually I can figure out an acronym by the context, but AAMOI I definitely had to look up – Carson Myers Jan 12 '11 at 13:46
  • @Carson - did you find anything? – Eimantas Jan 12 '11 at 14:38
  • 4
    As A Matter Of Interest – Carson Myers Jan 13 '11 at 14:25
  • The alternative for `_a()` invoking `b()` would be `_a()` invoking `_b()`, and `b()` just a public wrapper for `_b()`. This really boils down to how well defined the definition for what `b()` should do is, and when changing `b()`, do you want `_a()` to change its behavior accordingly. –  May 17 '12 at 16:19

10 Answers10

33

No not bad smell. This might be needed, why do you suspect it to be wrong ? A method at an atomic level is an independent entity that does a task. As long as it does a task anyone who has access to it can call it to get the task done.

Geek
  • 3,951
  • 1
  • 24
  • 29
  • I agree. Some methods provide functionality that is useful both for internals, and for external callers. A red flag might be if the public method was very specific to the internal implementation, but even then, not every class is intended to hide its internals. I often have a data structure "tool" layer underneath a container abstract type layer, for instance - that way I can re-use some of the data structure code for other data structures underlying other containers. –  Jan 12 '11 at 12:00
21

Code smell? Yes, not a really bad one, but a good indicator that the class may have too many responsibilities.

Take it as a sign that the class may need to be broken up into different objects, private methods shouldn't really need to call public methods of the same object, certainly in a clean OO design.

Of course, once you've inspected the class and the reasons for the method call are clear, it may be a perfectly reasonable use, in general you'd expect utility methods for the class to be private, but if one is useful enough to be public and utilised by other methods, I would, generally, expect those methods to be public also.

As with all code smells, this is a motivation for further code inspection, rationalise and maybe refactor, but not a cause for alarm.

ocodo
  • 2,948
  • 3
  • 23
  • 31
  • 5
    +1 for 'not a cause for alarm'. Too many times we see a 'smell' and instantly go into refactor mode without thinking. – Michael K Jan 12 '11 at 15:13
  • +1 for SRP. I just ran into a case where a decorator wasn't working because the decorated class was calling its own public function, bypassing the decorator. The solution: break the class into two and inject the decorated dependency. – Jon Hulka Aug 06 '18 at 03:10
18

It might lead to unpleasant surprises if someone who hasn't read the source code of this class tries to subclass it and overrides the public method. Whether that is a real concern obviously depends on your situation. Maybe you should consider making the public method or even the class final.

Kim
  • 1,135
  • 1
  • 7
  • 14
  • Why? If an overwritten public method leads to unpleasant surprises, does it really matter who called the method? – user281377 Jan 12 '11 at 11:16
  • 4
    Yes it does. If overriding one public method breaks another public method I can override that other method too. If it breaks a private method, I can ....? – Kim Jan 12 '11 at 11:57
  • 5
    An unpleasant surprise resulting from overriding a public method is a code smell in itself. A code stench, actually. – Larry Coleman Jan 12 '11 at 13:41
  • Kim: if the method is public, you must assume that other classes call that method just as well... – user281377 Jan 12 '11 at 13:44
  • @Larry: Exactly! A private method (which usually contains implementation-specific assumptions) calling a public one makes it more likely that overriding the public method would break things. – Kim Jan 14 '11 at 09:51
  • I agree with the comments above that calling a virtual method from within a private method isn't an issue. This is one way for a class to offer delegated functionality (though I don't personally design classes in this way, it's common.) Also, the poster was generally concerned with public methods. Public methods aren't intrinsically virtual (in all languages? most languages?) – Sprague Jun 12 '13 at 07:36
5

I dont think we can genaralize.

It all depends very heavily on context.

I might have, say, a public utility method in a class which is used by other classes, and also some private method in the same class.

Nivas
  • 526
  • 2
  • 5
5

No. What else should be done in that case? Make the private method public or the public method private? Copy-Paste the code from the public method to the private one?

user281377
  • 28,352
  • 5
  • 75
  • 130
  • Or have the public method delegate to a (new) private method which is invoked by the other private method. But, why bother? – Lawrence Dol Jan 12 '11 at 23:42
4

NO, no bad smell here.

if we implement interface of a queue with List, is it a bad smell to just call the proper List functions in order to achieve the implementation of queue easily?

if you have something and you want to convert it to something else(like wrapper) then its not a bad smell, its code re-usability with design pattern acted in function level(is a function an object?)

Display Name
  • 769
  • 4
  • 13
2

I know this is an old post, but it's something I've been debating at work. I 'do' consider this a code smell, and can't understand why you would ever want to do this. If a private method must call a public method, then the content of the public method should be taken and placed in a private method, which both methods can then call. Why?

  1. The public method may contain tests that aren't necessary once your executing code internally. It might receive a UserObj and want to test user permissions for example.

  2. After a public call, you may have a requirement to lock the object down, if you're using threading, so internally, you wouldn't want to call back out to a public method.

  3. More likely to introduce circular errors and infinite loops and out of mem exceptions in my opinion.

  4. Plain and simply, bad design and "lazy". Public methods provide access to the outside world. There's no reason to walk back outside when you're already inside.

Mark Graham
  • 21
  • 2
  • 1
    What if the existing public method is called by many other classes? Making it private will break that. Remember that public methods are called by other classes for other reasons, not just this class. kinda why public methods exist. – Michael Durrant Jan 18 '17 at 12:29
  • I stated that 'the content of the public method should be taken and placed in a private method...' The public method will remain, but make a call to the new private method. As will any other private methods that need to re-use the same functionality. – Mark Graham Jan 18 '17 at 17:15
  • So you are adding another method for no other reason than - well, because you declared this to be a "code smell". Pointless methods is a worse code smell. – gnasher729 Jan 18 '17 at 19:56
  • Sorry, but, did I not just list several reasons above. A lot of the code I write is for business systems, many classes are restricted to users based upon role. Many public methods test user permissions and params. Why on earth would I want to call back out to the same public method to reuse its functionality and re-test the user permissions? – Mark Graham Jan 18 '17 at 23:41
  • I have never in my life needed to call a public method from a private one. It just seems and feels very wrong to me. I'm really surprised most people think it's ok. – Trap Nov 27 '18 at 22:54
2

Imagine the opposite. You are in a private method and you need functionality in a public method What if you couldn't call that public method from the private one. What would you do?

  • Create a duplicate private method? No
  • Create a special public method for the occassion? No
  • Call a special private method that can call public methods? No
  • Some sort of super that can do this? No

The answer is clearly that when you want the functionality in a public method you should be able to call that method from methods of this class or from other classes.

Michael Durrant
  • 13,101
  • 5
  • 34
  • 60
1

The question you need to ask yourself is why does your class have the same need as clients of your class? Usually a class has very different needs from its clients. So yes, this is an indication that you have either

(a) exposed something publicly that should be private; or

(b) the class's behavior isn't narrow enough (think single responsibility principle).

Bradley Thomas
  • 5,090
  • 6
  • 17
  • 26
1

In my code I often create lazy load getters, which is to say the object gets initialized the first time it's requested and thereafter reuses the same instantiated object. However, an object instantiated using a lazy load implies that it may not necessarily be instantiated at any given point. Rather than wrap my head around the sequence of calls such that I know that that object is already instantiated or repeating the same code of a lazy load inside another method, I simply call the lazy loader whenever I need that object.

Just as you can use public methods in a smart way, you can also use them incorrectly. An example of this might be a public method which processes its parameters before calling another private method. It would be a mistake to call that public method casually simply because you have the same parameters. The mistake is subtle but it's a design error more than anything else and requires that you learn to manage with the parameters of the internal method rather than the parameters of the public method.

So to answer your question, it's certainly not bad code if you use it correctly.

Neil
  • 22,670
  • 45
  • 76