312

There is a lot of code in one of our projects that looks like this:

internal static class Extensions
{
    public static string AddFoo(this string s)
    {
        if (s == null)
        {
            return "Foo";
        }

        return $({s}Foo);
    }
}

Is there any explicit reason to do this other than "it is easier to make the type public later?"

I suspect it only matters in very strange edge cases (reflection in Silverlight) or not at all.

bopapa_1979
  • 8,949
  • 10
  • 51
  • 76
  • 1
    In my experience, that's the normal practice. – phoog Feb 15 '12 at 22:09
  • 6
    @phoog, OK. but why is it the normal practice? Maybe just easier then changing a bunch of methods to internal? Quick fix --> mark the type internal instead? – bopapa_1979 Feb 15 '12 at 22:11
  • that's what I've always assumed. I don't have a well-thought-out analysis, however, which is why I didn't post an answer. – phoog Feb 15 '12 at 22:13
  • 2
    Eric Lippert's answer sums up my thinking very well, and also brings up something that was lurking in my brain trying to find a way out: implicit implementations of interface members must be public. – phoog Feb 15 '12 at 22:51
  • Isn't that method equal to `return s + "Foo";`? The `+` operator doesn't care about null or empty strings. – Mikkel R. Lund Jan 05 '16 at 22:33

12 Answers12

507

UPDATE: This question was the subject of my blog in September 2014. Thanks for the great question!

There is considerable debate on this question even within the compiler team itself.

First off, it's wise to understand the rules. A public member of a class or struct is a member that is accessible to anything that can access the containing type. So a public member of an internal class is effectively internal.

So now, given an internal class, should its members that you wish to access in the assembly be marked as public or internal?

My opinion is: mark such members as public.

I use "public" to mean "this member is not an implementation detail". A protected member is an implementation detail; there is something about it that is going to be needed to make a derived class work. An internal member is an implementation detail; something else internal to this assembly needs the member in order to work correctly. A public member says "this member represents the key, documented functionality provided by this object."

Basically, my attitude is: suppose I decided to make this internal class into a public class. In order to do that, I want to change exactly one thing: the accessibility of the class. If turning an internal class into a public class means that I have to also turn an internal member into a public member, then that member was part of the public surface area of the class, and it should have been public in the first place.

Other people disagree. There is a contingent that says that they want to be able to glance at the declaration of a member and immediately know whether it is going to be called only from internal code.

Unfortunately, that doesn't always work out nicely; for example, an internal class that implements an internal interface still has to have the implementing members marked as public, because they are part of the public surface of the class.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 15
    I like this answer... it fits well with my attitude towards writing code that is self documenting to the greatest extent possible, and scope is a great way to broadcast intent, especially if your other team members understand your convention. – bopapa_1979 Feb 15 '12 at 22:51
  • 11
    +1 for saying what I wanted to say but formulating it much better than I was able (also for bringing up the interface angle, though I note that it's only true of implicit member implementations). – phoog Feb 15 '12 at 22:53
  • 2
    The Interface part is important - it's impossible to implement an interface method using internal, it's either public or explicit interface declaration. So the word public is overloaded with two meanings. – Michael Stum Feb 15 '12 at 23:12
  • 9
    From an idealogical perspective your argument in favor of `public` is very convincing. However, I find the "doesn't always work out nicely..." to be especially powerful. Losing consistency devalues any "at-a-glance" benefits. Using `internal` in this way means deliberately building intuitions that are sometimes wrong. Having a mostly-right intuition is IMO a horrible thing to have for programming. – Brian Feb 15 '12 at 23:19
  • 3
    Important quote from your blog post: "My advice is to discuss the issue amongst your team, make a decision, and then stick to it." – bopapa_1979 Mar 15 '17 at 17:10
  • Does this mean you would never have a internal member in a internal class? Of are there situations where a specific member of the class should stay internal if you later make the class itself public? – comecme May 08 '20 at 08:17
23

If the class is internal, it doesn't matter from an accessibility standpoint whether you mark a method internal or public. However it is still good to use the type you would use if the class were public.

While some have said that this eases transitions from internal to public. It also serves as part of the description of the method. Internal methods typically are considered unsafe for unfettered access, while public methods are considered to be (mostly) free game.

By using internal or public as you would in a public class, you ensure that you are communicating what style of access is expected, while also easing the work required to make the class public in the future.

Guvante
  • 18,775
  • 1
  • 33
  • 64
  • I tend towards limiting everything to the greatest extent possible while creating a good API and allowing my intended consumer to do what I intend them to do. I'm also with you on marking the member as I would if the class were public. More specifically, I'd only mark any member public if i consider it always safe. Otherwise somebody else marking the class public later (it happens) could expose non-safe members. – bopapa_1979 Feb 15 '12 at 22:41
  • @Eric: If you want to forgo determining the safeness of a method until you are ready that is fine, but I was referring only to methods deemed safe, in which case there is no exposing. – Guvante Feb 15 '12 at 23:16
11

I suspect that "it is easier to make the type public later?" is it.

The scoping rules mean that the method will only be visible as internal - so it really doesn't matter whether the methods are marked public or internal.

One possibility that comes to mind is that the class was public and was later changed to internal and the developer didn't bother to change all the method accessibility modifiers.

Oded
  • 489,969
  • 99
  • 883
  • 1,009
11

I often mark my methods in internal classes public instead of internal as a) it doesn't really matter and b) I use internal to indicate that the method is internal on purpose (there is some reason why I don't want to expose this method in a public class. Therefore, if I have an internal method I really have to understand the reason why it's internal before changing it to public whereas if I am dealing with a public method in an internal class I really have to think about why the class is internal as opposed to why each method is internal.

Ɖiamond ǤeezeƦ
  • 3,223
  • 3
  • 28
  • 40
  • 1
    Thanks for the answer. I tend to stubbornly insist that "everything" matters when coding, though :) – bopapa_1979 Feb 15 '12 at 22:53
  • 2
    You are correct Eric, it was a bit of a throw away comment. Hope that the rest of my answer was of some use. I think Eric Lippert's answer is what I was trying to express but he explained it so much better. – Ɖiamond ǤeezeƦ Feb 16 '12 at 10:41
9

In some cases, it may also be that the internal type implements a public interface which would mean that any methods defined on that interface would still need to be declared as public.

Trevor Pilley
  • 16,156
  • 5
  • 44
  • 60
2

For the same reason as using public methods in any other class - so that they're public to the outside of the containing type.

Type's access modifier has exactly zero to do with its members' access modifiers. The two decisions are made completely independently.

Just because certain combinations of type and members' modifiers produce seemingly (or as others call it "effectively") the same result doesn't mean they're semantically the same.

Local access modifier of a an entity (as declared in code) and its global effective access level (as evaluated through the chain of containment) are completely different things, too. An open office inside of a locked building is still open, even though you can't really enter it from the street.

Don't think of the end effect. Think of what you need locally, first.

  • Public's Public: classic situation.
  • Public's Internal: type is public but you want some semi-legal access in the assembly to do some hacky-wacky stuff.
  • Internal's Public: you hide the whole type but within the assembly it has a classic public surface
  • Internal's Internal: I can't think of any real world example. Perhaps something soon to become public's internal?

Internal's Public vs Internal's Internal is a false dilemma. The two have completely different meaning and should be used each in their own set of situations, non-overlapping.

Stepan Stulov
  • 21
  • 5
  • Yet another way to think about this. I got what I needed from the accepted answer, but this is useful, too. "Think of what you need locally, first." Roger that. – bopapa_1979 Sep 28 '21 at 15:30
2

It's the same, the public method will be really marked as internal since it's inside a internal class, but it has an advantaje(as you guested), if you want to mark the class as public, you have to change fewer code.

Mario Corchero
  • 5,257
  • 5
  • 33
  • 59
  • 3
    there is already a related question: http://stackoverflow.com/questions/711361/public-vs-internal-methods-on-an-internal-class – Mario Corchero Feb 15 '12 at 22:13
0

I actually struggled with this today. Until now I would have said that methods should all be marked with internal if the class was internal and would have considered anything else simply bad coding or laziness, specially in enterprise development; however, I had to sub class a public class and override one of it's methods:

internal class SslStreamEx : System.Net.Security.SslStream
{
    public override void Close()
    {
        try
        {
            // Send close_notify manually
        }
        finally
        {
            base.Close();
        }
    }
}

The method MUST be public and it got me thinking that there's really no logical point to setting methods as internal unless they really must be, as Eric Lippert said.

Until now I've never really stopped to think about it, I just accepted it, but after reading Eric's post it really got me thinking and after a lot of deliberating it makes a lot of sense.

Storm
  • 1,848
  • 4
  • 20
  • 39
0

There does be a difference. In our project we have made a lot of classes internal, but we do unit test in another assembly and in our assembly info we used InternalsVisibleTo to allow the UnitTest assembly to call the internal classes. I've noticed if internal class has an internal constructor we are not able to create instance using Activator.CreateInstance in the unit test assembly for some reason. But if we change the constructor to public but class is still internal, it works fine. But I guess this is a very rare case (Like Eric said in the original post: Reflection).

Farrah Jiang
  • 73
  • 8
0

I think I have an additional opinion on this. At first, I was wondering about how it makes sense to declare something to public in an internal class. Then I have ended up here, reading that it could be good if you later decide to change the class to public. True. So, a pattern formed in my mind: If it does not change the current behavior, then be permissive, and allow things that does not makes sense (and does not hurt) in the current state of code, but later it would, if you change the declaration of the class.

Like this:

public sealed class MyCurrentlySealedClass
{
    protected void MyCurretlyPrivateMethod()
    {
    }
}

According to the "pattern" I have mentioned above, this should be perfectly fine. It follows the same idea. It behaves as a private method, since you can not inherit the class. But if you delete the sealed constraint, it is still valid: the inherited classes can see this method, which is absolutely what I wanted to achieve. But you get a warning: CS0628, or CA1047. Both of them is about do not declare protected members in a sealed class. Moreover, I have found full agreement, about that it is senseless: 'Protected member in sealed class' warning (a singleton class)

So after this warning and the discussion linked, I have decided to make everything internal or less, in an internal class, because it conforms more that kind of thinking, and we don't mix different "patterns".

Hix
  • 29
  • 3
  • I drastically prefer to do it the way (or at least for the reasons) Eric Lippert said. His article is definitely worth a read. – bopapa_1979 May 03 '18 at 16:32
0

It's not relevant for your example, but for classes where interfaces being implemented in C# up to 7.3 inclusive it is:

internal interface IAmOnlyInternalViewable
{
    void CallMeOnlyInsideAssembly();
    //internal void CallMeOnlyInsideAssembly();  // <-- is not possible up to C# 7.3 inclusive
}

internal class OnlyInternalVisible : IAmOnlyInternalViewable
{
    public void CallMeOnlyInsideAssembly() { } //Must be public to implement the interface
}

So if you want to have an internal class, which implements an interface, the implemented methods need to be public.

Rekshino
  • 6,954
  • 2
  • 19
  • 44
0

internal says the member can only be accessed from within the same assembly. Other classes in that assembly can access the internal public member, but would not be able to access a private or protected member, internal or not.

kitti
  • 14,663
  • 31
  • 49
  • But if methods were marked `internal` instead of `public`, their visibility wouldn't change. I believe that's what the OP is asking about. – Oded Feb 15 '12 at 22:13
  • 1
    @zapthedingbat - the post is factually correct, but is it actually answering the question? – Oded Feb 15 '12 at 22:17
  • 1
    Right, the question is WHY mark them that way. I understand the basics of scope. Thanks for the attempt, though! – bopapa_1979 Feb 15 '12 at 22:42