Break. It. Up! – Private Method Access Modifiers as Code Smells

There’s a couple of anti-patterns that I’ve noticed in my own code.

Private methods are a code smell

Private methods are not inherently bad, but they are a sign that you might be missing out on an opportunity to make a useful abstraction. If you have a private method that calls another private method, then there is almost certainly an area of responsibility that remains unidentified.

If I have a class with a private method, then it’s probably the result of me refactoring a piece of a public method, but not taking it to conclusion – I should keep going and move the method its own class!

I move a private method into its own class, and I am forced to think about its dependencies and API. Now it’s automatically testable and re-composable.

Use cases:

• Private method with no dependencies – just move the method to a new class
• Private method with dependencies on services – move the method to a new class, rationalize its dependiencies, use instances as a dependency of the parent.
• Private method that uses mutates state – move the method to a new class, state mutation commands delivered from the method and interpreted by the parent.

Too many public methods are a code smell

When I have a class with a load of public instance methods, it’s unlikely that they all operate on the same state, or depend on the same objects.

If the methods don’t operate on the same thing, then they don’t belong together.

When I am thinking about the dependencies that I need to create an object (e.g. for testing), if all the dependencies are not used by all the methods, then maybe a red light should be flashing in my brain.

I see this a lot in ASP.NET MVC controller, where I have a controller that is notionally about a class of entity, but the sets of method depend on different things (database access, report outputs, email). Break it up.