Should methods with business logic be made private? [duplicate]

Question

So I am writing a project using Spring Boot. All of my logic resides in @Service classes. I have separated each service class based on entity. For example - If I have two independent entities A and B, then each has following different classes:

1. RestController
2. DTOs
3. Service
4. Repository
5. Entity

When I start writing code for a feature, it starts with RestController (an api call basically), which calls a method of service class. This method is public and initially is very small, just input and output. This makes it easy to test (unit tests).

@Service
public AService {
  public AResponseDto rootMethod(SomeChunkyDto someChunkyDto) {
    return new AResponseDto();
  }
}

However, as I add more logic to the method, it becomes bulkier and unreadable.

public AResponseDto rootMethod(SomeChunkyDto someChunkyDto) {
  // Logic 1
  // Logic 2
  // Logic 3
  // Logic 4
  AResponseDto aResponseDto = // Logic 5
  return aResponseDto;
}

So, I refactor:

private String helperMethod1(/* Less chunky, small arguments - preferably primitive data types */) {
       // Logic 1
      // Logic 2
}

private String helperMethod2(/* Less chunky, small arguments - preferably primitive data types */) {
       // Logic 3
      // Logic 4
}
public AResponseDto rootMethod(SomeChunkyDto someChunkyDto) {
  helperMethod1(...args);
  helperMethod2(...args);
  AResponseDto aResponseDto = // Logic 5
  return aResponseDto;
}

Of course, the actual code is larger and complex. I tried to simplify.

So here is the issue - Those private methods contain logic that should be tested. As they are private, the only way to test them is by testing rootMethod. This makes things complicated, as rootMethod has a bulky input. It is time consuming.

Another thing I have read is that methods with logic shouldn't be private. That way they can be tested very easily. But then they are accessible to other classes which have no use of it.

Helper methods which contain logic that can be reused in other classes, I make them public. But most of the helper methods are specific to a particular entity and their use is limited to that particular service class.

I am not sure what the right approach is here. Should I make the private methods public? Currently I am testing them via rootMethod. But then it is not much of a unit test is it?

Edit: A friend of mine suggested that I should create a separate class for helper methods, like AHelper which will contain helperMethod1 and helperMethod2. That way methods can be public and they can be accessed in @Service class by injecting AHelper as a dependency.

Not sure if this is correct way to do as methods are public and accessible by classes that don't require them. Another thing is that the project is quite small, most probably won't scale in future. So I am trying to keep less files (classes).

Answer 1

Sprout and Split

You've got Sprouting down pat. You've already moved business logic into its own functions. Hopefully while you were doing this you were deduplicating and generalising the code too.

The next trick is to split this class into two or more classes. Your friend put it as making a Helper class, I find that terminology unhelpful. Its like a class called Utility - Yes I presumed it would be usable. This is more like making the missing class that actual governs a piece of Logic, and then giving it a good name.

You will find that some of those private functions use specific variables, either passed in, or in the class body itself. This speaks of a cohesive unit of logic. They probably belong to the same class, try splitting them off into a collaborator, and given it a more helpful name than Helper.

Some of those functions might actually be variants of the same pattern of behaviour with some hooks that change it slightly. See if you cannot refactor them down to one version with a number configurable arguments. This function would then be a good candidate for refatoring into a method class so that the configuration options are specified in the class constructor, leaving the function itself to accept the data that is actually being processed.

Answer 2

First, it's OK to level up method visibility to make method testable. Java has not only private/public modifiers. I've read in many sources advice put test classes to the same package so you could test protected and package-private (default) methods without any efforts. So if I were you I would change private String helperMethod1(...) to

/** method has default access modifier due to test reasons. Don't use it outside this class! */
String helperMethod1(...)

Comment in this case is essential: it prevents from changing access level in occasion ('Hey, some asshole forget public word, now I'll fix it').
Also I suppose you have tests somewhere like my.company.controllers.AServiceTest and AService is in my.company.controllers package.
Now you could add tests for helperMethod1 in AServiceTest and don't worry that someone would use it.

---

When people introduce AHelper with (static) methods without state, they often downshift from object-oriented to procedural programming. You have to ask questions:

• Does AHelper have its own state?
• Do any other classes call methods from AHelper?
• Is AService abnormally big? (more than 500 lines, although this number varies from project to project)

If for all questions you answer 'no' then you don't need to introduce AHelper class.

Answer 3

The service class is the right place for the business logic

public/private is about whether the method should be called from outside the object, business logic has no impact on that choice.

Avoid "Helper" classes. This is a vague instruction, but essentially its a bad thing if you have a class with a bunch of unrelated methods which is in turn referenced from all over the code base.

So

ProcessXClass
    RunProcessX()

is fine if you service is getting big and unwieldy, but..

HelperMethods
    RunProcessXSubMethod()
    RunProcessYSubMethod()

is bad