22

This question already has an answer here:

I've always used public methods and recently been advised by one of my friends to always avoid defining methods as public, as far as possible, though I have worked in any commercial company I have never "really" understood the concept of why public methods are not "safe to use at times".

I have worked a lot with c# and please don't get me wrong I have used other access types such internal, private and protected and I know how they "work" but never really understood why you should define methods with those access levels, especially in production.

|improve this question

marked as duplicate by gnat, Doc Brown, Andres F., 8bittree, amon Nov 8 '17 at 9:44

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 5
    "using public methods" defining methods as public or calling methods that are public? – Caleth Nov 7 '17 at 18:41
  • @Caleth, in this context I meant defining the methods as public – Sid Khanye Nov 7 '17 at 19:17
  • 22
    In addition to the good answers you are getting, I'd like to point out that it is bad software practice to "always use public methods," and you can be certain of that because it has the word "always" in it. As you continue in your development career, you'll find good practices with "always" in them are few and far between. – Cort Ammon Nov 7 '17 at 19:37
  • 1
    @TheJoker are you writing library code for other teams to use, or code for just your own team? Most of the answers below have the assumption that people outside your team are going to use it, and expect new versions as requirements change. – Carl Walsh Nov 7 '17 at 21:35
  • 3
    @ThorbjørnRavnAndersen In Java you should not find that many classes need to be subclassed, and the classes that do need to be designed specifically to be subclassed. – Solomonoff's Secret Nov 7 '17 at 21:38
59

The methods, properties and constructors (i.e. members of a class) that you define using a public accessor determine the surface area that the users of your class will be allowed to interact with. Any method that you don't want to be part of this surface area should not be made public.

Reasons why some members might not be public:

  1. They are implementation details; they implement behavior needed by the public members, but are not meant to be called from the outside directly (i.e. private).
  2. They are meant to be accessed in a derived class, but not called directly. (i.e. protected).

Think of it in terms of your house. You don't let the tax man come into your house, find your checkbook and write himself a check. Rather, the tax man sends you a bill, you review the bill, write a check and send it to him. The sending of the bill and the check are both public acts. The acts of reviewing the bill and writing the check are implementation details performed in the privacy of your own home that the tax man doesn't need to know anything about.

Further Reading
Encapsulation

|improve this answer
39

Short answer:

Declaring a method or field "public" translates to a promise:

Dear colleages,

I invite you to use this method or field wherever you find it appropriate.

I have documented everything you need to know about its usage, and as long as you don't violate this documentation, I take responsibility for every bug in this context.

I promise that I'll never change the behaviour of this method / interpretation of the data in this field, or I'll analyze all of your code using my method and change it appropriately.

Are you sure you want that (especially the "documentation" part)?

|improve this answer
  • 9
    Good luck getting any of those promises. public makes one promise, and one promise only; you can call it from the outside, today. – Robert Harvey♦ Nov 7 '17 at 19:38
  • 14
    I know the sad reality from 30+ years of software development. Developers change public methods without thinking about the consequences, they don't document their methods, even the method naming doesn't help to understand what's going on. It's time to change that! – Ralf Kleberhoff Nov 7 '17 at 19:46
  • I've worked in both kinds of contexts, where a lack of documented behavior either means "this method promises only to do what I (the author) expect", or "this method promises to keep returning the same output for every input". If you don't define what counts as "garbage" parameters/calls/etc. then it's hard to argue garbage-in, garbage-out. – Carl Walsh Nov 7 '17 at 21:32
  • 15
    Instead the promise should be: Dear Colleagues, I invite you to use this method wherever appropriate. Example usage is in the unit tests. Feel free to add whatever unit tests you feel appropriate to codify your assumptions. We will never change the method behavior in a unit test failing way without analyzing our code and changing appropriately. – emory Nov 7 '17 at 22:55
  • 3
    Depending on how widely the code is used and how much else depends on it, either @emory's or Ralf's promise may be implied, or possibly a weaker promise. The point is that there is some promise with public that is completely absent with private. – Carl Leth Nov 8 '17 at 1:22
10

If I have a class:

class SomeThing
{
    public DoSomething()
    {
        DoThing1();
        DoThing2();
    }

    public DoThing1() ...
    public DoThing2() ...
}

Now I might intend everyone to call DoSomething only. But I've made DoThing1 and DoThing2 public too, so people might still use them. As a result, when I later rewrite DoSomething to use DoThing3 only, I'm stuck: I can't get rid of DoThing1 and DoThing2, even though I'm no longer using them.

If I'd marked them as private though, then no such problem would arise. I can safely delete them without breaking external code.

So a good rule of thumb is mark everything private unless it absolutely needs to be accessed outside the class. If it must be accessed, mark it internal if possible as it still remains "private" to that project and can be changed without breaking other projects. Only if it is needed as part of the public API of the project should you mark it public. This applies as much to classes, enums etc as it does to members of those classes etc.

|improve this answer
  • On the flip side, failure to make DoThing1 and DoThing2 public may result in client code that needs to perform one without the other having use its own implementation of DoThing1; if the behavior of DoThing1 changes, such code would no longer be consistent with the behavior of invoking DoThing1 from within DoSomething. – supercat Nov 7 '17 at 21:49
  • 3
    @supercat, if client code needs access to just DoThing1, then, at that stage, make it public. It's easy to move things from restricted access to less restricted. It's going the other way that causes problems. So always start with the most restrictive. – David Arno Nov 7 '17 at 21:58
  • Making things less restrictive may be easy if clients can simply call up the programmer and say "Hey, can you make XX public". Oftentimes, however, the author of client code will have no direct communication with the author of the code being called. – supercat Nov 7 '17 at 22:02
  • 5
    @supercat If you have ever worked on very large, public API's, you will find that the technique you recommend does not scale. Once you make something public, your options for making changes become more limited. Private, internal, then public is always the preferred order, unless you work alone. – Frank Hileman Nov 7 '17 at 22:12
  • @DavidAmo I agree, however some organisations are very slow. Take a large ecommerce framework designed to be highly flexible. It is expected that when I want to make custom functionality, I can extend base classes, and if the default were "Private first, public at request", I could almost never use those classes because getting a method changed to public could take months. As an implementor with clients and deadlines, that can't work. So I guess as always, "It depends" is applicable. – Ben Nov 8 '17 at 5:21
7

Classes expose an interface (the English word, not the c# keyword), through which you use them.

Make interfaces easy to use correctly and hard to use incorrectly.

If your class has a function that should not be called from other classes, yet you allow other classes to do so, you make it easy to use the class incorrectly. This introduces bugs, maintenance costs and training costs. This isn't as bad if you're the sole developer - you already know the code and know which functions you can or cannot call. Unless you have to revisit your code 6 months later, at which point you can't remember.

|improve this answer
1

If you're writing "large-scale" software (i.e. working with multiple teams, releasing new versions over time, have enough code that it's hard to keep it all in your head at once), then yes it's a bad engineering practice (see the other great answers).

If you're just writing one-off code (e.g. a single-file script to push data around with just 3 methods) then it's overkill.

|improve this answer
  • 1
    It might be overkill in one-off code, but one-off code often grows to become multiple use code, and good habits should be trained at all times to make them better habits. – CodeMonkey Nov 8 '17 at 8:29
  • 2
    I agree that knowing how to write maintainable code is a good skill to learn, but it's also valuable to know when not to over-engineer your code. If there's no scenario or requirement that your class be called by another code, you could make every method public or private and it wouldn't really matter--if you don't know how your code will be used, versioned, serviced, etc. then your design will likely need to change. – Carl Walsh Nov 8 '17 at 8:58
0

The access specifiers also act as a kind of documentation/comment/description. When you read some source code, if you see something private you know immediately that the thing is only used internally. This helps improve code readability.

|improve this answer

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