Twitter GitHub Facebook Instagram dirv.me

Daniel Irvine on building software

Internal methods on public classes are a code smell

1 February 2013

In the .NET world, the abuse of internal methods on public classes is fairly common, and it’s especially a problem if you’re building a framework. What I’m talking about is this:

// it's a public class, accessible to other assemblies
public class MyType
{
    // this method can be accessed by other assemblies
    public void HappyMethod()
    {
        BadMethod();
    } 

    // this method can be called by other types in this assembly, 
    // but not other types in other assemblies
    internal void BadMethod()
    {
       // do stuff
    }
}

To clarify, the internal keyword is used to signal that it is callable by the same type and by other types in the same assembly, but not types in other assemblies.

First let me assume that no method starts life as internal: it’s either public, called when the callee wants the class to perform its job, or it’s private, called as a result of one of the public methods within the class being called:

public class MyType {
    public void HappyMethod () { BadMethod(); }
    void BadMethod() {  /* not so bad yet */ }
}

Now MyType is happily doing it’s thing, completed and working according to the specifications. You’re busy writing writing AnotherType which is using HappyMethod. For whatever reason, you need to use BadMethod from AnotherType:

class AnotherType {
   public void SomeMethod() {
      var myType = new MyType(...);
      // ... 
      myType.BadMethod();  // won't work, BadMethod is private
} 
}

To the untrained eye, your first thought will be “I’ll make this method internal; it’s still private to the outside world so nothing has really changed.”

Solving the problem this way solves the problem for you but not for others. Anyone else trying to use your class and runs in to this problem will now be shafted because you chose to make the method internal.

Cohesion is of our guiding principles as software engineers. One meaning of cohesion is that all the state and behaviour of your class works together in a tight unit; you can’t use one ‘half’ of the class without using the other ‘half’ of the class (and if you found yourself with two separate halves then the right thing to do would be to split that class into two).

As a client of a class you should expect the class to be cohesive and as a result you will also expect to follow a similar usage pattern to all other clients of that class. In other words, you’ll possibly instantiate the class, set a few inputs, perform an operation and then collect the outputs (all of those done by calling methods and accessing properties). If this class were part of a framework, that usage pattern would probably be documented in the user’s guide.

Your class now has one usage pattern that works for its own assembly but is prohibited for all other assemblies. That severely hampers use of the class.

To go back to the original problem, there are two better solutions:

  1. Refactor the class into two; place BadMethod in an internal class as a public method. This doesn’t help your poor users, so it should only be done if it increases cohesion. If both of these classes will need to be used in unison, then your classes are now coupled and your users are still screwed. Worse, you might find yourself duplicating state between the two classes. However, often the reason for wanting
  2. Make the method public instead. Problem solved, at the expense of increasing your public API.
Another way of looking at this is that class is king. The “assembly” is just a deployment unit. If you think about it like that, why should types within the same assembly have more privilege than types outside of the assembly?  C++ has the friend keyword which is more appropriate; true, it increases coupling but in a very limited scope compared to internal.

As a final thought, if you have an internal class and you want to make a private method internal, save yourself the hassle and make the method public. Same effect, but you’re being clearer about your intentions.

About the author

Daniel Irvine is a software craftsman at 8th Light, based in London. These days he prefers to code in Clojure and Ruby, despite having been a C++ and C# developer for the majority of his career.

For a longer bio please see danielirvine.com. To contact Daniel, send a tweet to @d_ir or use the comments section below.

Twitter GitHub Facebook Instagram dirv.me