It's not an enviable experience to arrive to new project team and see methods violating all rules of common sense. Sometimes it's because of programmers ignorance, sometimes because of "yesterday deadline", sometimes because some people like to be irreplaceable (and by producing code maintenable with difficulty, they can achieve that in some way). This post tries to justify why short methods are better than the long ones.
The form of this post is unusual. It contains 1 section trying to justify that the fact that writing small methods is good.
These reasons are:
- Single Responsability Principle (SRP)
The SRP states that every object and method should be responsible for only 1 specific thing. Most than often, long methods are sources of violation of this principle. Since they're long, even very long, they have a natural tendency to handle many operations.
SRP violation leads often to God anti-pattern (aka Monster objects) where one specific object (method in described case) knows everything and, accordingly, isn't reusable and testable. - method inlining
Since Java is compiled to machine code at runtime, it allows some runtime optimizations. One of them is method inlining. It occurs when given method reaches some configured threshold of calls and the compiler decides to put its content inside the caller. But there are some conditions for that. One of them is the frequency of calls - more often method is called, more it's susceptible to be inlined. The second condition is related to the subject of this post - method size. It's configured through -XX:MaxInlineSize=SIZE argument. One more reason to keep methods small.
You can obtain further information on this subject from the article about code inlining. - readability
Obviously, smaller method is more readable. Not only because the scope of logic to caught is smaller but also because it has something that long method body doesn't have - naming context. Let's compare below 2 methods:
private void buyItem(List<Item> items) { // Check if all items are in stock int defaultStockQuantity = 5; items.stream().filter(item -> defaultStockQuantity >= item.quantity).findFirst() .ifPresent((item) -> { throw new IllegalStateException("Items out of stock"); }); User user = new User(); double reduction = 0d; if (user.reputation == 100) { reduction = 0.2d; } else if (user.reputation > 50) { reduction = 0.1d; } else if (user.reputation > 10) { reduction = 0.02d; } // Calculate final price double itemsPrice = items.stream().map(item -> item.price) .reduce((a, b) -> a + b).get(); double reductionPrice = itemsPrice * reduction; double orderPrice = itemsPrice - reductionPrice; // And some other code following above verbose logic } private void buyItemRewritten(List<Item> items) { if (isOutOfStock(items)) { throw new IllegalStateException("Items out of stock"); } double userReduction = resolveUserReduction(new User()); double orderPriceWithReduction = calculateOrderPrice(items, userReduction); // And some other code } private static class User { private int reputation; }The reading of the buyItemRewritten() gives a feeling of reading a book chapter - we can simply read some action, do not focus on implementation details, until reaching the point of code that interests us. In the other side, buyItem() is more complicated - almost every time it requires some interpretation. Worst, once time screen height passed, we lose the context. Context recovery is more difficult than in the case of methods giving some narrativity. For example, by seeing order price we can forget about the fact that it's a price containing reduction (especially when the variable is badly named as in the example). - testability
Testability is another point in favor of short methods. Let's retake code implemented previously but written against Spring component-oriented framework:
@Test private void should_fail_because_on_item_is_out_of_stock() { StockService mockedStockService = mock(StockService.class); when(mockedStockService.allItemsAreInStock(anyList())).thenReturn(false); // make the test } // tested methods private void buyItemService(List<Item> items) { boolean allItemsInStock = stockService.allItemsAreInStock(items); if (!allItemsInStock) { // fail... } } private interface StockService { boolean allItemsAreInStock(List<Item> items); }Above example shows testing for short method. Because of logical decomposition (that obviously leads to its short form), the test is almost limited on mocking responses of injected services. The verbose buyItem() method couldn't be testable as easy as it. In order to check items availability, we should create appropriated objects, the same to compute user's reduction and so on. With shorter method, we can simply mock services.
- reusability
More focused responsability given method has, easier it can be reused. Let's take below code coming from a "lambda party" project:
"Lambda party"
As "lambda party" I consider a project where programmers were too excited about lambdas introduction to Java 8 and have started to overuse them, as in following snippet:
orders.filter(order -> order.getQuantity() > 10) .map(order -> { ValuableOrder valuableOrder = new ValuableOrder(); valuableOrder.setProducts( order.getProducts().stream().map(product -> { valuableProduct valuableProduct = new valuableProduct(); valuableProduct.setName(product.getName()); valuableProduct.setQuantity(product.getQuantity()); valuableProduct.setUnitPrice(product.getUnitPrice()); return valuableItem; }).collect(Collectors.toList()); ); }.forEach(valuableOrder -> { EmailTemplate emailTemplate = new EmailTemplate(valuableOrder); if (!valuableOrder.getDeliveryCountry().isEu()) { mailService.sendEmail(valuableOrder.customer(), taxesInfo); } else { emailTemplate = emailTemplate.translate(Locale.ENGLISH); } mailService.sendEmail(valuableOrder.customer(), emailTemplate); valuableOrder.getProducts().forEach(product -> { itemService.decreaseProductQuantity(product.getQuantity()); }); });private void orderItems(List<Item> items) { int universalQuantity = 5; // check if all items are available items.stream().filter(item -> universalQuantity >= 5).findFirst() .ifPresent((item) -> { throw new IllegalStateException("Order can't be passed for items out of stock"); }); // do the rest } private void sendOrder(Object order, List<Item> items) { int universalQuantity = 5; List<Item> outOfStockItems = items.stream().filter(item -> universalQuantity >= 5).collect(Collectors.toList()); // Do something with out of stock items }As you can see, filtering operation is duplicated in every method. With customized filter method (or object), it can be reused and tested simpler:private void orderItemsRewritten(List<Item> items) { int universalQuantity = 5; // check if all items are available items.stream().filter(item -> isInStock(item, universalQuantity)).findFirst() .ifPresent((item) -> { throw new IllegalStateException("Order can't be passed for items out of stock"); }); // do the rest } private void sendOrderRewritten(Object order, List<Item> items) { int universalQuantity = 5; List<Item> outOfStockItems = items.stream().filter(item -> isInStock(item, universalQuantity)).collect(Collectors.toList()); // Do something with out of stock items } private static boolean isInStock(Item item, int stock) { return stock >= item.quantity; }As you can see, the logic of main methods remains the same. But unlike previously, filtering part is shared among them and thus is reusable easier.
This post presents some of reasons in favor of short methods. Of course, the notion of "short" should be always thought in pragmatic terms and sometimes it can be violated in favor of other concepts. But at least the list above gives some starting points for reflection about the question: "Does my method is too long ?".
