7 Answers
7

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.

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.
–
Steve314Jan 12 '11 at 12:00

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.

Why? If an overwritten public method leads to unpleasant surprises, does it really matter who called the method?
–
user281377Jan 12 '11 at 11:16

2

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 ....?
–
KimJan 12 '11 at 11:57

3

An unpleasant surprise resulting from overriding a public method is a code smell in itself. A code stench, actually.
–
Larry ColemanJan 12 '11 at 13:41

Kim: if the method is public, you must assume that other classes call that method just as well...
–
user281377Jan 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.
–
KimJan 14 '11 at 9:51

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.

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.