Sergey Nivens - Fotolia
Use Java method modifiers to avoid public method antipattern
Follow best practices for Java method design, and don't allow one public class to call another. If you skipped this step, be prepared to pay the consequences.
While it seems so basic that it wouldn't require a mention, there's a basic and often violated software design principle pertaining to Java method modifiers that is in need of explanation.
A public method should never be called by another public method within the same class hierarchy. It should only be called by other classes via the classes' public interface.
Class calls might not be the first and foremost software programming design principle, but it seems to be the one that I see broken most often. Fast-paced environments require a developer to get things done as quickly as possible and result in this principle often being overlooked for speed instead of clarity. As a result, when a use case pops up later on and requires the same public method from another class, we can run into problems. Another issue that can arise is that it becomes more difficult for a developer to refactor classes to a better design.
You can quickly and easily correct this Java method modifier issue based on how soon you can refactor. Then, it just comes down to which method you want to use.
Let's explore two options on how to fix this issue and diagnose the pros and cons of each selection.
Java method modifiers example
To demonstrate your options as simply as possible, let's start with a simple code example. I will use Spring Framework with @Transactional to demonstrate why a public method that calls another public method in the same class can create such a big problem. @Transactional is how Spring will add transactions to your code via proxies. These proxies will play a major role in how this call method issue will arise. In this code example, the OrderService interface's saveOrder method brings up the main problem.
public interface OrderService {
public Order findOrder(long id);
public Order saveOrder(Order order);
public void updateCustomersOrdersAsShipped(long custId);
}
@Service
@Transactional
public class OrderServiceImpl implements OrderService {
private OrderRepository orderRepository;
@Autowired
public OrderServiceImpl(OrderRepository orderRepository) {
this.orderRepository = orderRepository;
}
@Override
public Order findOrder(long id) {
return orderRepository.findOne(id);
}
@Transactional(propagation=Propagation.REQUIRES_NEW)
@Override
public Order saveOrder(Order order) {
return orderRepository.save(order);
}
@Override
public void updateCustomerOrdersAsShipped(long custId) {
List<Order> ordersForCustomer = orderRepository.findAllOrdersForCustomer(custId);
for (Order customerOrder: ordersForCustomer) {
boolean isReadyForShipment = validateOrderReadyForShipment(customerOrder);
customerOrder.setShipDate(new LocalDate());
//
Here, we now have a public method calling another public method within the same class:
this.saveOrder(customerOrder);
}
}
private boolean validateOrderReadyForShipment(Order order) {
boolean readyForShipment = true;
if (order.getReadyForShipment != null) {
readyForShipment = false;
}
return readyForShipment;
}
}
You will notice that we have updateCustomerOrderAsShipped -- a public method in the interface or class -- making a call to another public method saveOrder() in the same class. This can be problematic for a few reasons. The saveOrder method wants to have a brand new transaction created every time it gets called. However, because Spring adds transactionality via aspect-oriented programming proxies, the call from updateCustomerOrderAsShipped won't create a new transaction. Instead, it will just participate in the already existing transaction because the call originates from inside the same class.
Imagine if you flew to Hawaii and received a lei. But once you're in Hawaii, you can't get another lei unless you leave the state and come back.
Fix the public method problem
There are a couple approaches to consider how to rectify the Java method modifier misuse issue.
A developer can move the shared code into a private method that both public methods call or move one of the public methods into a different class and let one class call the other.
But which one is the better choice to create transaction demarcation? The first choice still needs a call to the first public method to start a transaction and the next call would be to the second public method to start another new transaction. A developer must find a way to get outside the Spring-created proxy, and that can't happen between a call within the same class. The only way this can occur is if the two methods are in two different classes and both get their own proxy. Also, one class will need to call the other and then go through the proxy of the called class.
Essentially, the first public method gets called from outside that class and starts a new transaction. Then, inside that class, it calls to another class through its proxy and creates the new REQUIRES_NEW transaction that we expected. I will create a new interface called CreateOrderService.
public interface CreateOrderService {
public Order saveOrder(Order order);
}
And an implementation public class CreateOrderServiceImpl implements CreateOrderService:
{
private OrderRepository orderRepository;
@Autowired
public CreateOrderServiceImpl(OrderRepository orderRepository) {
this.orderRepository = orderRepository;
}
@Transactional(propagation=Propagation.REQUIRES_NEW)
@Override
public Order saveOrder(Order order) {
return orderRepository.save(order);
}
}
With this new interface and class, OrderServiceImpl will now look like this:
public interface OrderService {
public Order findOrder(long id);
public void updateCustomersOrdersAsShipped(long custId);
}
@Service
@Transactional
public class OrderServiceImpl implements OrderService {
private CreateOrderService createOrderService;
private OrderRepository orderRepository;
@Autowired
public OrderServiceImpl(OrderRepository orderRepository, CreateOrderService createOrderService) {
this.orderRepository = orderRepository;
this.createOrderService = createOrderService;
}
@Override
public Order findOrder(long id) {
return orderRepository.findOne(id);
}
@Override
public void updateCustomerOrdersAsShipped(long custId) {
List<Order> ordersForCustomer = orderRepository.findAllOrdersForCustomer(custId);
for (Order customerOrder: ordersForCustomer) {
boolean isReadyForShipment = validateOrderReadyForShipment(customerOrder);
customerOrder.setShipDate(new LocalDate());
//
Here, we now have a public method calling another public method within the same class:
createOrderService.saveOrder(customerOrder);
}
}
private boolean validateOrderReadyForShipment(Order order) {
boolean readyForShipment = true;
if (order.getReadyForShipment != null) {
readyForShipment = false;
}
return readyForShipment;
}
}
What are some tradeoffs with the two-class approach?
The biggest issue I've come across when addressing the Java method modifier issue focuses on application complexity with circular references among classes. For example, Class A references Class B, which references Class C, which, in turn, references Class A. As a result, a developer will need to untangle the circular references as Spring makes proxies and determine which class needs to be created first to properly set up the links. Other potential tradeoffs include extra classes in code bases and determining which methods need to be moved to other classes.
When should we create a private shared method option?
In most cases, I believe the first option will be your best choice to fix public method issues and will always be the easiest to implement.
My approach is to take the code you need moved to the private method and declare the private method as simply private void sharedCode() {...}.
At this point, you'll see some errors in the new private method because parameters haven't been set, and some variables or parameters from the public method will need to be passed to this new method to enable the code to compile. Use the red in your IDE to help you determine the parameters you need to pass in. I prefer to see red errors instead of a guess-and-check approach to create the private method -- unless it's a small private method, in which case it can be simple to identify the problem areas.
In most cases, the private method will need more parameters that are harder to determine upfront. In our code example, it's easy to see that the only parameter we will need is the Order object. The only line in saveOrder is orderRepository.save(order), and that also happens to be the only line that will be in our new private method.
private Order saveOrderPrivate(Order order) {
return orderRepository.save(order);
}
And the public methods now look like this:
@Transactional(propagation=Propagation.REQUIRES_NEW)
@Override
public Order saveOrder(Order order) {
return this.saveOrderPrivate(order);
}
@Override
public void updateCustomerOrdersAsShipped(long custId) {
List<Order> ordersForCustomer = orderRepository.findAllOrdersForCustomer(custId);
for (Order customerOrder: ordersForCustomer) {
boolean isReadyForShipment = validateOrderReadyForShipment(customerOrder);
customerOrder.setShipDate(new LocalDate());
this.saveOrderPrivate(customerOrder);
}
}
Pitfall of this approach
One tradeoff to be aware of is code access. The code we moved into the private method will only be available to that class. If you want to share it with other classes, you will no longer have this access. You could potentially create an interface method with the same signature to pass all parameters down to the private method and return what it returns to fix this issue. In the case of our example, the saveOrder method would do just that. But, in many real-world scenarios, the code would be much more complex.
