Is it okay for a class to use its own public method?

Question

Background

I currently have a situation where I have an object that is both transmitted and received by a device. This message has several constructs, as follows:

public void ReverseData()
public void ScheduleTransmission()

The ScheduleTransmission method needs to call the ReverseData method whenever it is called. However, there are times where I will need to call ReverseData externally (and I should add outside the namespace entirely) from where the object is instantiated in the application.

As for the "receive" I mean that ReverseData will be called externally in an object_received event-handler to un-reverse the data.

Question

Is it generally acceptable for an object to call its own public methods?

Answer 1

I would say it's not only acceptable but encouraged especially if you plan to allow extensions. In order to support extensions to the class in C#, you would need to flag the method as virtual per the comments below. You might want to document this, however, so that someone isn't surprised when overriding ReverseData() changes the way ScheduleTransmission() works.

It really comes down to the design of the class. ReverseData() sounds like a fundamental behavior of your class. If you need to use this behavior in other places, you probably don't want to have other versions of it. You just need to be careful that you don't let details specific to ScheduleTransmission() leak into ReverseData(). That will create problems. But since you are already using this outside of the class, you probably have already thought that through.

Answer 2

Absolutely.

The visibility of a method has a sole purpose to allow or deny access to a method outside the class or within a child class; public, protected and private methods can always be called inside the class itself.

There is nothing wrong in calling public methods. The illustration in your question is the perfect example of a situation where having two public methods is exactly what you should do.

However, you may carefully watch for the following patterns:

1. A method Hello() calls World(string, int, int) like this:

   Hello()
   {
       this.World("Some magic value here", 0, 100);
   }
   

   Avoid this pattern. Instead, use optional arguments. Optional arguments make discoverability easier: the caller who types Hello( won't necessarily know that there is a method which makes it possible to call the method with default values. Optional arguments are also self-documenting. World() doesn't show to the caller what are the actual default values.

2. A method Hello(ComplexEntity) calls World(string, int, int) like this:

   Hello(ComplexEntity entity)
   {
       this.World(entity.Name, entity.Start, entity.Finish);
   }
   

   Instead, use overloads. Same reason: better discoverability. The caller can see at once all the overloads through IntelliSense and pick the right one.

3. A method simply calls other public methods without adding any substantial value.

   Think twice. Do you really need this method? Or should you remove it and let the caller invoke the different methods? A hint: if the name of the method doesn't seem right or is difficult to find, you should probably remove it.

4. A method validates input and then calls the other method:

   Hello(string name, int start, int end)
   {
       if (name == null) throw new ArgumentNullException(...);
       if (start < 0) throw new OutOfRangeException(...);
       ...
       if (end < start) throw new ArgumentException(...);
   
       this.World(name, start, end);
   }
   

   Instead, World should validate its parameters itself, or be private.

Answer 3

If something is public, it might be called at any time by any system. There's no reason you can't be one of those systems too!

In highly optimized libraries, you want to copy the idiom put forth in java.util.ArrayList#ensureCapacity(int).

ensureCapacity

• ensureCapacity is public
• ensureCapacity has all necessary bounds checking and default values, etc.
• ensureCapacity calls ensureExplicitCapacity

ensureCapacityInternal

• ensureCapacityInternal is private
• ensureCapacityInternal has minimal error checking because all inputs come from inside the class
• ensureCapacityInternal ALSO calls ensureExplicitCapacity

ensureExplicitCapacity

• ensureExplicitCapacity is ALSO private
• ensureExplicitCapacity has no error checking
• ensureExplicitCapacity does actual work
• ensureExplicitCapacity is not called from anywhere except by ensureCapacity and ensureCapacityInternal

In this way, code you trust gets privileged (and faster!) access because you know its inputs are good. Code you don't trust goes through certain rigor to verify its integrity and bomb or provide defaults or otherwise handle bad inputs. Both of them channel to the the place that actually does work.

However, this is used in ArrayList, one of the most used classes in the JDK. It's very possible your case doesn't require that level of complexity and rigor. Long story short, if all of the ensureCapacityInternal calls were replaced with ensureCapacity calls, the performance would still be really, really good. This is a microoptimization probably only made after extensive consideration.

Answer 4

Several good answers have been given already, and I would also agree with them that yes, an object may call its public methods from its other methods. However, there is a slight design caveat that you'll have to watch out for.

Public methods usually have a contract of "take the object in a consistent state, do something sensible, leave the object in a (possibly different) consistent state". Here, "consistent" may mean, for example, that the Length of a List<T> is no greater than its Capacity and that referencing the elements at indices from 0 to Length-1 won't throw.

But inside the object's methods, the object may be in an inconsistent state, so when you call one of your public methods, it may do a very wrong thing, because it wasn't written with such a possibility in mind. So if you plan to call your public methods from your other methods, make sure that their contract is "take the object in some state, do something sensible, leave the object in a (possibly different) (maybe inconsistent—but only if the initial state was inconsistent) state".

Answer 5

Another example when calling public method inside another public method is totally fine is an CanExecute/Execute approach. I use it when I need both validation and invariant preservation.

But in general I'm always cautious about that. If a method a() is called inside method b(), it means that method a() is an implementation detail of b(). Very often it indicates that they belong to different abstraction levels. And the fact that they both are public makes me wonder whether it's a Single-responsibility principle violation or not.

Answer 6

Sorry but im going to have to disagree with most of the other 'yes you can' answers and say that:

I would discourage a class calling one public method from another

There are a couple of potential issues with this practice.

1: Infinite loop in inhertited class

So your base class calls method1 from method2 but then you, or someone else, inherits from it and hides method1 with a new method which calls method2.

2: Events, Logging etc.

eg I have a method Add1 which fires an event '1 added!' I probably don't want the Add10 method to raise that event, write to a log or whatever, ten times.

3: threading and other deadlocks

Eg InsertComplexData opens a db connection, starts a transaction, locks a table, Then calls InsertSimpleData, with opens a connection, starts a transaction, waits for the table to be unlocked....

I'm sure there are more reasons, one of the other answer touched on 'you edit method1 and are surprised method2 starts behaving differently'

Generaly if you have two public methods which share code, its better to make them both call a private method rather than one call the other.

Edit ----

Lets expand on the specific case in the OP.

we dont have a lot of detail but we know that ReverseData is called by an event handler of some kind as well as the ScheduleTransmission method.

I asume that reverse data also changes the internal state of the object

Given this case I would think that thread saftey would be important and hence my third objection to the practice applies.

To make ReverseData thread safe you can add a lock. But if ScheduleTransmission also needs to be thread safe you will want share the same lock.

The easiest way of doing this is to move the ReverseData code into a private method and have both public methods call it. You can then put the lock statement in the Public Methods and share a lock object.

Obviously you can argue "that will never happen!" or "I could program the lock another way" but the point about good coding practice is to structure your code well in the first place.

In academic terms I would say this violates the L in solid. Public methods are more than just publicly accesible. They are also modifiable by its inheritors. Your code should be closed for modification which means you have to think about what work you do in both public and protected methods.

Heres another one : You also potentialy violate DDD. If your object is a domain object its public methods should be Domain terms which mean something to the business. In this case its very unlikely that 'buy a dozen eggs' is the same as 'buy 1 egg 12 times' even if it starts off that way.