23

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?

  • 5
    There is nothing wrong with calling a public method from its own. But, based on your method names, ReverseData() sounds to me a bit dangerous to be a public method if it does reverse the internal data. What if ReverseData() called outside of the object and then called again with the ScheduleTransmission(). – Kaan Mar 14 '16 at 19:28
  • @Kaan These aren't the real names of my methods, but closely related. In fact "reverse data" only reverses 8 bits of the total word and is done when we receive and transmit. – Snoop Mar 14 '16 at 19:31
  • The question still stands. What if those 8 bits are reversed twice before the transmission is scheduled? This feels like a huge hole in the public interface. Thinking about the public interface like I mention in my answer might help bring this problem to light. – Daniel T. Mar 14 '16 at 21:31
  • 2
    @StevieV I believe this only amplifies the concern Kaan raises. It sounds as thought you're exposing a method that changes the state of the object, and the state depends primarily on how many times the method is called. This makes for a nightmare in trying to keep track of what the object's state is throughout your code. It sounds to me more like you would benefit from separate data types from these conceptual states, so you can tell what it is in any given piece of code without having to worry about it. – jpmc26 Mar 15 '16 at 1:30
  • 2
    @StevieV something like Data and SerializedData. Data is what the code sees and SerializedData is what gets sent through the network. Then make reverse data (or rather serialize/deserialize data) transform from one type to the other. – csiz Mar 15 '16 at 10:08
33

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.

  • Wow, seemed really odd to me but this helps. Would like to also see what some other people say. Thank you. – Snoop Mar 14 '16 at 17:23
  • 2
    "so that someone isn't surprised when overriding ReverseData() changes the way ScheduleTransmission() works": no, not really. At least not in C# (note that the question has a C# tag). Unless ReverseData() is abstract, no matter what you do in the child class, it's always the original method which will be called. – Arseni Mourzenko Mar 14 '16 at 19:27
  • 2
    @MainMa Or virtual... And if you're overriding the method, then by definition it needs to be virtual or abstract. – Pokechu22 Mar 14 '16 at 19:56
  • 1
    @Pokechu22: indeed, virtual works too. In all cases, the signature, according to OP, is public void ReverseData(), so the part of the answer about overriding stuff is slightly misleading. – Arseni Mourzenko Mar 14 '16 at 20:10
  • @MainMa you're saying that if a base class method calls a virtual method it doesn't behave in the usual virtual manner unless it was abstract? I looked up answers on abstract vs virtual and did not see that mentioned: rather abstract just means there is no version given innthis base. – JDługosz Mar 15 '16 at 0:00
20

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.

  • Would the same thoughts apply if ReverseData was actually internal, and used throughout the namespace containing instances of "object"? – Snoop Mar 14 '16 at 17:35
  • 1
    @StevieV: yes, of course. – Arseni Mourzenko Mar 14 '16 at 19:28
  • @MainMa I don't understand what's the reason behind points 1 and 2 – Kapol Mar 15 '16 at 8:14
  • @Kapol: thank you for your feedback. I edited my answer by adding explanation about the first two points. – Arseni Mourzenko Mar 15 '16 at 8:39
  • Even in case 1, I usually prefer overloads rather than optional arguments. If the options are such that using overloads is not an option or produces especially large numbers of overloads, I would create a custom type and use that as the argument (as in suggestion 2), or refactor the code. – Brian Mar 15 '16 at 13:27
8

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.

3

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".

1

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.

-5

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.

  • 3
    These issues sound more like poorly thought-out architecture than a general condemnation of the design principle. (Maybe calling "1 added" is fine every time. If it isn't, we should program differently. Maybe if two methods are trying to exclusive-lock the same resource, they shouldn't call each other, or we wrote them poorly.) Rather than present bad implementations, you may want to focus on more solid arguments that tackle accessing public methods as a universal principle. For example, given good neat code, why's it bad? – doppelgreener Mar 15 '16 at 1:53
  • Exactly, if it isn't, you dont have a place to put the event. Similarly with #3 everything is fine untill one of your methods down the chain needs a transaction, then you are screwed – Ewan Mar 15 '16 at 2:04
  • All of these problems are reflective more of the code itself being poor quality than of the general principle being flawed. All three cases are just bad code. They can be resolved by some version of "don't do that, do this instead" (perhaps with asking "what do you want exactly?"), and don't call into question the general principle of a class calling its own public methods. The vague potential for an infinite loop is the only one here that gets to the matter as a matter of principle, and even then isn't tackled thoroughly. – doppelgreener Mar 15 '16 at 2:27
  • the only thibg the 'code' in my example shares is one public method calling another. If you think its 'bad code' good! That is my contention – Ewan Mar 15 '16 at 8:23
  • That you can use a hammer to break your own hand doesn't mean the hammer is bad. It means you need to not do things with it that will break your hand. – doppelgreener Mar 15 '16 at 9:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.