Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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?

share|improve this question
3  
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 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. – StevieV Mar 14 at 19:31
1  
@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 at 1:30
1  
@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 at 10:08
1  
@RobertHarvey Hmm... Sort of... But I took the "overengineered" comment personally :) I am not doing it that way anymore. In truth it was overengineered. I've decided to just have a static class with the 4 or 5 parse methods I need, and then call them on an as-needed basis. That will work for me, thank you. – StevieV Mar 17 at 16:11
up vote 30 down vote accepted

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.

share|improve this answer
    
Wow, seemed really odd to me but this helps. Would like to also see what some other people say. Thank you. – StevieV Mar 14 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. – MainMa Mar 14 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 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. – MainMa Mar 14 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 at 0:00

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.

share|improve this answer
    
Would the same thoughts apply if ReverseData was actually internal, and used throughout the namespace containing instances of "object"? – StevieV Mar 14 at 17:35
1  
@StevieV: yes, of course. – MainMa Mar 14 at 19:28
    
@MainMa I don't understand what's the reason behind points 1 and 2 – Kapol Mar 15 at 8:14
    
@Kapol: thank you for your feedback. I edited my answer by adding explanation about the first two points. – MainMa Mar 15 at 8:39
    
@MainMa Thank you for clarifying. +1 – Kapol Mar 15 at 9:00

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.

share|improve this answer

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

share|improve this answer

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.

share|improve this answer
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 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 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 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 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 at 9:20

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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