Methods should not perform too many tasks (Brain Method).
Why is this an issue?
This issue is raised when Sonar considers that a method is a 'Brain Method'.
A Brain Method is a method that tends to centralize its owner’s
class logic and generally performs too many operations. This can include checking too many conditions, using lots of variables, and ultimately making
it difficult to understand, maintain and reuse.
It is characterized by high LOC number, high cyclomatic and cognitive complexity, and a large
number of variables being used.
What is the potential impact?
Brain Methods are often hard to cover with tests, because of their deep nesting, and they are error-prone, because of the many local variables they
usually introduce. Such methods will be very hard to read and understand for anyone outside who created them, and therefore hard to maintain and fix
if bugs get spotted.
They also enable code duplication since the method itself can hardly be reused anywhere else.
How to fix it
The common approach is to identify fragments of the method’s code that deal with a specific responsibility and extract them to a new method. This
will make each method more readable, easy to understand and maintain, easier to test, and more prone to be reused.
In this paper, the authors
describe a systematic procedure to refactor this type of code smell: "Assessing the Refactoring of
Brain Methods".
Code examples
Noncompliant code example
void farmDailyRoutine() {
Crops southEastCrops = getCrops(1, -1);
Crops eastCrops = getCrops(1, 0);
WaterContainer waterContainer = new WaterContainer();
List<Bottle> bottles = new ArrayList<>();
for(int i = 0; i < 10; i++) {
var bottle = new Bottle();
bottle.addWater(10L);
bottle.putCap();
bottle.shake(2);
bottles.add(bottle);
}
waterContainer.store(bottles);
Truck t1 = new Truck(Truck.Type.TRANSPORT);
t1.load(waterContainer);
if(Weather.current != Weather.RAINY) {
WaterContainer extraWaterContainer = new WaterContainer();
List<Bottle> extraBottles = new ArrayList<>();
if(southEastCrops.isDry()) {
for(LandSlot ls : southEastCrops.lands()) {
Bottle b = new Bottle();
b.addWater(10L);
b.putCap();
extraBottles.add(b);
}
} else {
extraBottles.add(new Bottle());
}
if(eastCrops.isDry()) {
for(LandSlot ls : southEastCrops.lands()) {
Bottle b = new Bottle();
b.addWater(10L);
b.putCap();
extraBottles.add(b);
}
} else {
extraBottles.add(new Bottle());
}
extraWaterContainer.store(extraBottles);
t1.load(extraWaterContainer);
} else {
WaterContainer extraWaterContainer = WaterSource.clone(waterContainer);
t1.load(extraWaterContainer)
}
}
Compliant solution
void farmDailyRoutine() { // Compliant: Simpler method, making use of extracted and distributed logic
Crops southEastCrops = getCrops(1, -1);
Crops eastCrops = getCrops(1, 0);
WaterContainer waterContainer = new WaterContainer();
List<Bottle> bottles = getWaterBottles(10, 10L, true);
waterContainer.store(bottles);
Truck t1 = new Truck(Truck.Type.TRANSPORT);
t1.load(waterContainer);
if(Weather.current != Weather.RAINY) {
WaterContainer extraWaterContainer = new WaterContainer();
fillContainerForCrops(extraWaterContainer, southEastCrops);
fillContainerForCrops(extraWaterContainer, eastCrops);
t1.load(extraWaterContainer);
} else {
WaterContainer extraWaterContainer = WaterSource.clone(waterContainer);
t1.load(extraWaterContainer)
}
}
private fillContainerForCrops(WaterContainer wc, Crops crops) { // Compliant: extracted readable and reusable method
if(crops.isDry()) {
wc.store(getWaterBottles(crops.lands().size(), 10L, false));
} else {
wc.store(Collections.singleton(new Bottle()));
}
}
private List<Bottle> getWaterBottles(int qt, long liquid, boolean shake){ // Compliant: extracted readable and reusable method
List<Bottle> bottles = new ArrayList<>();
for(int i = 0; i < qt; i++) {
Bottle b = new Bottle();
b.addWater(liquid);
b.putCap();
if(shake) {
b.shake();
}
bottles.add(b);
}
return bottles;
}
How does this work?
In this case, the method farmDailyRoutine was taking care of performing many different tasks, with nested conditions and loops, it was
long and had plenty of local variables. By separating its logic into multiple single-responsibility methods, it is reusing parts of its original
duplicated code and each of the new methods is now readable and easy to understand. They are now also easier to cover with tests, and many other parts
of the owner class could benefit from using these methods.
Resources
Articles & blog posts
