tags:

views:

654

answers:

12

There are lots of times where I'm not sure whether a particular method should be made private or not. For example, I'm building a class right now, which, is responsible for generating a report. This class has a buildReport method and several methods which collect the necessary data for buildReport.

// single public method
// uses a set of helper methods
public buildReport()

// helper methods
private avgSurveyTime()
private fetchVendors()
private fetchSendCounts()
private ...

I'm debating whether I should make these helper methods public. The only method I really plan on calling outside at the moment is buildReport(). However, it might be useful to get just a list of the vendors with fetchVendors() etc.

I see two schools of thought on this: You can always expose as little as possible. (In which case, many of my classes would only have one public method) OR you can expose all you can that might be useful to the user of the class.

Is there a good rule of thumb to use for deciding when methods should be made public/private?

+50  A: 

The only rule I follow is to make as little as possible public.

Look at it this way. You can always make something public later on - it won't break any existing code. Trying to make something private that was public could well end up breaking a lot of existing code.

If someone wants more functionality from your class then they can make the request and you can expose what they need. Chances are that they'll want something that's subtly different to what you already have anyway so you'll need a new interface.

ChrisF
I like this approach. This seems to be the general consensus among most of the answers.
AaronSzy
Exactly, give them what they need and nothing more.
Jamie Keeling
+1 Perfect answer, these were exactly my thoughts.
Helper Method
+1, with an additonal point - if you're about to make something public that was private, you need to think about why, and see if maybe the thing that's accessing this method should be delegating more responsibility to that class, so it wouldn't need the private method in the first place. Private methods are usually plumbing code, and if you're reaching for the plumbing, you're out of your element.
Tesserex
+1. At the expense of sounding like 'me too', I agree.
George Stocker
A rule that I also follow is to keep a VERY low number of private methods, because they can usually extracted to another class as public to be unit tested.
bloparod
+3  A: 

If it's not needed outside the class it is in at that very moment, make it private. If later it is, you can make it protected or public as needed.

When in doubt - make it private.

Daniel Bingham
+8  A: 

A helpful guidance technique to follow is to write an interface for your class before you start the implementation. Then when implementing, tell yourself that a method not in the interface should be private, or not in that class at all.

If you didn't know a method needed to be there when you were designing the contract of the class, it probably shouldn't be part of its public interface.

Ben James
This is good idea. Ill give this a try sometime.
AaronSzy
+1  A: 

The rule is that a method should be made provided unless it is needed. One of the main reasons for this is that in a future release of an API etc., you can always make a private function public, but you can almost never make a previous public function private without breaking existing code.

Kevin
+6  A: 
  1. You should expose to the outside world only what the outside world really needs. It is often best to add functionality for consumers of the class when it is needed, rather than at first. These days the wisdom is to avoid pre-engineering. (see YAGNI)

  2. It can certainly be acceptable to have public methods that are also used by other functionality within the class. However, this should be considered a minor bad smell. It might be an indication that your class is trying to do too many things.

My guess is to leave your classes as they are. Then, when these other, smaller methods are needed by the outside world, consider if you should separate your classes. If the purpose of each of these classes to to yield one report, then you should not expose these methods from this object. Instead, put "smaller" methods into a common helper class. That way they are available to the outside world without fundamentally changing the nature of your existing report classes. In short:

Don't just do it because you think it might be helpful later. If/When the additional functionality is needed, reconsider your overall design to accommodate the new requirements.

Patrick Karcher
Thanks for the YAGNI link. Also, ill look out for times where i may be violating point 2. Using it as a smell makes a lot of sense.
AaronSzy
I find the concept of code smells is incredibly helpful.
Patrick Karcher
+2  A: 
Juha Syrjälä
+2  A: 

If the class is for internal use within your organization, i.e. you are not distributing this as a general-use library, then I heartily agree that you should make as much private or protected as possible. If later you discover that some other class needs access to a private function, fine, at that point change it to public. It's not hard to do.

My only caveat would be if this is something you are "publishing", where others who do not have a direct line to get changes made will be using it. Then you need to carefully think out a complete API.

But failing that, just write the code you need. Don't write functions that you think you might use someday.

Jay
That makes sense. Most of my code doesn't leave here. Also, there are only a few guys on our team so ill probably be the only one working on it anyway.
AaronSzy
+2  A: 

Public and private methods are very different beasts. Use caution before making a method public.

  • Public methods must validate all of their parameters.
  • They must be properly documented, including any exceptions that they might throw.
  • All edge cases must be analyzed and delt with (in code or in documentation).
  • Any requirements involving the order in which public methods are called must be documented or, preferably, removed.
  • Object state requirements must also be documented and validated.
  • Public methods must not change their signature or behavior in any way that may break an application when moving from one version to the next.
  • Public methods may need to be designed with marshalling requirements. (Such as .Net's CLS restrictions.)

Private methods have none of these limitations.

Jeffrey L Whitledge
Right, so basically public methods generally have greater maintenance costs. So, theres more incentive to avoid making something public unless its really necessary.
AaronSzy
A: 

Everything not included in the class's interface should (must, really) be private. If you're not sure what the class's interface is (i.e., you're not programming to interfaces or those interfaces aren't yet fully defined, start with everything private and make things public, protected, package-private, etc. as needed.

But think carefully! Once something is accessible to other code, there's a dependency between that code and this class and refactoring is constrained. Rule of thumb: Expose only those methods that define the abstraction, not how it's implemented.

Michael Junkin
+1  A: 

I always follow this: "Design for tomorrow, code for today."

If today you need only one public method then keep only one public method.

chikak
A: 

simple rules:

  1. If you are not looking to expose it outside of the class that is using it, make it PRIVATE.

  2. If you are looking to expose it inside the same assembly to other classes but not outside the assembly, make it INTERNAL (C#) / Friend (VB.NET).

  3. If you are looking to expose the functionality outside the assembly to everyone, make it PUBLIC

Israel Johnson
A: 

I am working on a system made up of mainly two things : importing data from various sources, and generating reports. The problem is that the whole thing was designed by someone lacking basic OO design skills. In the same 'class' they would have 'private' methods to read data from a database, calling 'private' methods to validate said data, which is also being called by another 'private' 500-line function duplicated more or less in the whole application which simply formats said data.

You should move away private avgSurveyTime() private fetchVendors() private fetchSendCounts() from the actual class which handles page layout.

Andrei Tanasescu
Ya, i have actually moved several of these methods to a data access layer (avgSurveyTime, getVendors, ...). There are some remaining private methods that so some additional processing on the data retrieved from the DAL. This report class basically just builds an abstraction of the report. There's a simple toJson method which uses a collaborator to convert that abstraction to json.I've debated moving the remaining private methods to another class, but it looks like that will just complicate things more than needed.
AaronSzy