CODEWITHSUNDEEP
javaunit-testing
Corey
Corey

Reputation: 35

Converting a public method into a private method

I recently refactored some code which converted a public method that was only being used in conjure with another public method, into one call.

public class service() {
  public String getAuthenticatedUserName() {
    return SecurityContext.getName();
  }

  public getIdentityUserIdByUsername(String username) {
    return db.getUser(username).getId();
  }
}

which was being utilised in a few other classes as service.getIdentityUserIdByUsername(service.getUsername()), which seemed redudant. A new method was created combining the two calls.

public getIdentityUserId() {
  return getIdentityUserIdByUsername(getUsername());
}

The getIdentityUserIdByUsername() is still being utilised in other classes without the need for getUsername(). However, the getUserName() method is no longer used in other classes.

My example is much simpler than the implementation, the method has test coverage that is a bit awkward to do (mocking static classes without Powermock and a bit of googling etc). In the future it's likely we will need the getUsername() method, and the method will not change.

It was suggested in code review that the getUsername() method should now be private due to it not being called anywhere else. This would require the explicit tests for the method be removed/commented out which seems like it would be repeated effort to rewrite or ugly to leave commented out code.

Is it best practice to change the method to private or leave it public because it has explicit coverage and you might need it in the future?

Upvotes: 1

Views: 1337

Answers (3)

Stephen C
Stephen C

Reputation: 718758

Is it best practice to change the method to private or leave it public because it has explicit coverage and you might need it in the future?

IMO, you are asking the wrong question. So called "best practice" doesn't come into it. (Read the references below!)

The real question is which of the alternatives is / are most likely to be best for you. That is really for you to decide. Not us.

The alternatives are:

  1. You could remove the test case for the private method.
  2. You could comment out the test case.
  3. You could fix the test case so that it runs with the private version of the method.
  4. You could leave the method as public.

To make a rational decision, you need to consider the technical and non-technical pros and cons of each alternative ... in the context of your project. But don't be too concerned about making the wrong decision. In the big picture, it is highly unlikely that making the wrong choice will have serious consequences.

Finally, I would advise to avoid dismissing options just because they are "code smell". That phrase has the same issue as "best practice". It causes you to dismiss valid options based on generalizations ... and current opinions (even fashions) on what is good or bad "practice".

Since you want someone else's opinion ("best practice" is just opinion!), mine is that all of the alternatives are valid. But my vote would be to leave the method as public. It is the least amount of work, and an unused method in an API does little harm. And as you say, there is a reasonable expectation that the method will be used in the future.

You don't need to agree with your code reviewer. (But this is not worth making enemies over ...)

References:

Upvotes: 3

Salim
Salim

Reputation: 2178

If the method is a public static then you can leave it as is because there is no impact of it being public. It is aside effect free method, it being exposed will never cause any harm.

If it is a object level public method then -

1) Keep it if it is like an API. It has well defined input, output and delivers a well defined functionality and has tests associated with it. It being public doesn't harm anything.

2) Make it private immediately if it has side effects. If it causes others methods to behave differently because it changes the state of the object then it is harmful being public.

Upvotes: 0

rzwitserloot
rzwitserloot

Reputation: 102832

It can make sense to want to test private methods. The industry standard way to do this, which has quite some advantages, is this:

  • Ensure that the test code lives in the same package as the code it tries to test. That doesn't mean same directory; for example, have src/main/java/pkg/MyClass.java and src/test/java/pkg/MyClassTest.java.
  • Make your private methods package private instead. Annotate them with @VisibleForTesting (from guava) if you want some record of this.

Separately from this, the entry space for public methods (public in the sense of: This is part of my API and defines the access points where external code calls my code) is normally some list of entrypoints.. if you have it at all. More often there is no such definition at all. One could say that all public methods in all public types implicitly form the list (i.e. that the keyword public implies that it is for consumption by external code), which then by tautology decrees that any public method has the proper signature. Not a very useful definition. In practice, the keyword public does not have to mean 'this is API accessible'. Various module systems (such as jigsaw or OSGi) have solutions for this, generally by letting you declare certain packages as actually public.

With such tooling, 'treeshaking' your public methods to point out that they need no longer be public makes sense. Without them... you can't really do this. There is such a notion as 'this method is never called in my codebase, but it is made available to external callers; callers that I don't have available here, and the point is that this is released, and there are perhaps projects that haven't even started being written yet which are intended to call this'.

Assuming you do have the tree-shaking concept going, you can still leave them in for that 'okay maybe not today but tomorrow perhaps' angle. If that applies, leave it in. If you can't imagine any use case where external code needs access to it, just delete it. If it really needs to be recovered, hey, there's always the history in version control.

Upvotes: 0

Related Questions