|
Hello Everyone!
Welcome to the 35th edition of It Depends. Hope you are all doing well and staying safe. My apologies for not sending out an edition last weekend - I got caught up with a few things and couldn't make time.
More than one person has reached out to me asking about how to structure code in methods. The question I hear most often is how much to break down a method. So let’s talk about how large a method should be.
Let’s say there’s a method in the codebase that does 4-5 things one after the other. The code for doing all those things is well-written, but the result is a somewhat large method. At what point should you break it up into multiple methods? There is the old adage about every method being fully visible without scrolling which I have always found weird. Let’s try to do better.
The good part about having all of the code inside a single method is that it is all in one place and in a sense, easy to absorb in one shot. The bad is part is that it makes the method large and if split, the reader will have to move in and out of multiple methods to understand what is going on.
I prefer the second approach because smaller methods are easier to understand for me. But there has to be a balance in that direction too. So while the correct answer is obviously “it depends”, I’ll try to give a little more in the way of guiding principles.
The key to writing or refactoring software is to reduce the cognitive load on the reader/maintainer of the code. All other considerations are subservient to that goal (of course, “it depends”). So the key to answering the question of granularity is to identify how people read and understand any piece of code. Or rather, how do we want them to read a piece of code.
Typically people read code from a higher level of abstraction to a lower level of abstraction. The guiding principle of each layer of code, typically represented by methods, is that it should prevent the user from wanting to dig deeper into the next layer. Not in the sense of sending them away screaming as quickly as possible, but rather by making what must be happening underneath so obvious that the reader never feels the need to read the next layer. In the context of this article, cognitive load can manifest in two forms: engagement and curiosity. We don’t want them to feel like they have to think (engagement) to understand something, and we don’t want them to want to think (curiosity) about something. This is sometimes called the “Principle of least astonishment” and applying this can help us bound the upper and lower granularity of a method.
At each level, a well-written method represents a declarative (what is to be done) unit to work to its higher layers (the ones who called it) and internally contains a set of imperatively defined steps (how is this to be done). In this sense, our method in question which does 4-5 things, is in some sense a workflow. As I wrote in my previous article on workflows, the place where a workflow is defined should only define what steps are to be taken, not how each of them works internally.
Let’s take the example of booking a doctor’s appointment at a hospital. A makeAppointment(hospitalId, doctorId, patientId, startTime, endTime) method can do several things:
- Check the working hours of the hospital
- Check if the doctor has no other appointment during that time
- Book the appointment
- Notify the doctor of the appointment
- Notify the patient of the appointment
Let implement this by putting all the code in one place (gist).
public Slot makeAppointment(Doctor d, Hospital h, Patient p, Date startTime, Date endTime) throws Exception {
// Check if hospital is open during this time period
String hospitalServiceBaseUrl = ConfigReader.readConfigName("hospitalServiceBaseUrl");
HospitalService hospitalService = new HospitalService(hospitalServiceBaseUrl);
DateRange range = hospitalService.getWorkingHours();
if (!range.contains(startTime) || !range.contains(endTime)) {
throw new Exception("Hospital isn't open in this time period");
}
// Check if doctor has no other appointment in this time period
String scheduleServiceBaseUrl = ConfigReader.readConfigName("scheduleServiceBaseUrl");
ScheduleSerivce scheduleService = new ScheduleService(scheduleServiceBaseUrl);
Slot s = scheduleService.getAppointmentSlots(d).stream()
.filter(slot -> !slot.isOccupied()) // Only consider free slots
.filter(slot -> slot.contains(startTime) && slot.contains(endTime)) // Only consider slots which contain this time period
.findFirst();
// Create the appointment
s.setIsOccupied(true);
Slot bookedSlot = scheduleService.bookSlot(d, p, s);
// Notify doctor using custom email template
DoctorEmailTemplate doctorEmailTemplate = new DoctorEmailTemplate(d, p, bookedSlot);
notificationService.notify(d, doctorEmailTemplate);
// Notify patient using custom email template
PatientEmailTemplate patientEmailTemplate = new PatientEmailTemplate(d, p, bookedSlot);
notificationService.notify(p, patientEmailTemplate);
return bookedSlot;
}
This method is not so bad as some real-world code you may have seen. But the moment I reach this method, my brain has to wake up to the details of everything going on here. I wanted to understand how an appointment is booked but suddenly I’ve run into a bunch of API calls and other things. This doesn’t exactly match my mental model of how to book an appointment. This result is a high cognitive load. Therefore, this method should be broken down till it reaches the mental model “in English” as much as possible. Here’s a possibility (gist).
public Slot makeAppointment(Doctor d, Hospital h, Patient p, Date startTime, Date endTime) throws Exception {
if (!isHospitalOpen(h, startTime, endTime)) {
throw new Exception("Hospital isn't open in this time period");
}
Slot s = getFreeSlotForDoctor(d, startTime, endTime);
if (s == null) {
throw new Exception("Doctor is not free in this time period");
}
Slot bookedSlot = createAppointment(s, d, p);
notifyDoctor(bookedSlot, d, p);
notifyPatient(bookedSlot, d, p);
return bookedSlot;
}
private boolean isHospitalOpen(Hospital h, Date startTime, Date endTime) {
String hospitalServiceBaseUrl = ConfigReader.readConfigName("hospitalServiceBaseUrl");
HospitalService hospitalService = new HospitalService(hospitalServiceBaseUrl);
DateRange range = hospitalService.getWorkingHours();
return range.contains(startTime) && range.contains(endTime);
}
private boolean getFreeSlotForDoctor(Doctor d, Date startTime, Date endTime) {
String scheduleServiceBaseUrl = ConfigReader.readConfigName("scheduleServiceBaseUrl");
ScheduleSerivce scheduleService = new ScheduleService(scheduleServiceBaseUrl);
return scheduleService.getAppointmentSlots(d).stream().filter(slot -> !slot.isOccupied) // Only consider free slots
.filter(slot -> slot.contains(startTime) && slot.contains(endTime)) // Only consider slots which contain this time period
.findFirst();
}
private Slot createAppointment(Slot s, Doctor d, Patient p) {
String appointmentServiceBaseUrl = ConfigReader.readConfigName("appointmentServiceBaseUrl");
AppointmentService appointmentService = new AppointmentService(appointmentServiceBaseUrl);
s.setIsOccupied(true);
return appointmentService.bookSlot(d, p, s);
}
private void notifyDoctor(Slot s, Doctor d, Patient p) {
DoctorEmailTemplate doctorEmailTemplate = new DoctorEmailTemplate(d, p, bookedSlot);
notificationService.notify(d, doctorEmailTemplate);
}
private void notifyPatient(Slot s, Doctor d, Patient p) {
PatientEmailTemplate patientEmailTemplate = new PatientEmailTemplate(d, p, bookedSlot);
notificationService.notify(p, patientEmailTemplate);
}
This looks a lot more like the workflow I was expecting. The details of each of the steps are now hidden, which means that they can change without us knowing about that. But the biggest thing is that I don’t feel like I have to go into each of the new methods to see what they do if all I want is a logical understanding of how appointments are booked. There are technical things like exceptions that still intrude upon the reading experience, but for the most part, astonishment has been eliminated by reducing the model-code gap.
Now let’s go one step deeper and further break down the methods here (gist).
public Slot makeAppointment(Doctor d, Hospital h, Patient p, Date startTime, Date endTime) throws Exception {
if (!isHospitalOpen(h, startTime, endTime)) {
throw new Exception("Hospital isn't open in this time period");
}
Slot s = getFreeSlotForDoctor(d, startTime, endTime);
if (s == null) {
throw new Exception("Doctor is not free in this time period");
}
Slot bookedSlot = createAppointment(s, d, p);
notifyDoctor(bookedSlot, d, p);
notifyPatient(bookedSlot, d, p);
return bookedSlot;
}
private boolean isHospitalOpen(Hospital h, Date startTime, Date endTime) {
String hospitalServiceBaseUrl = ConfigReader.readConfigName("hospitalServiceBaseUrl");
HospitalService hospitalService = new HospitalService(hospitalServiceBaseUrl);
DateRange range = hospitalService.getWorkingHours();
return range.contains(startTime) && range.contains(endTime);
}
private boolean getFreeSlotForDoctor(Doctor d, Date startTime, Date endTime) {
return buildScheduleService().getAppointmentSlots(d).stream().filter(slot -> !slot.isOccupied) // Only consider free slots
.filter(slot -> slot.contains(startTime) && slot.contains(endTime)) // Only consider slots which contain this time period
.findFirst();
}
private ScheduleService buildScheduleService() {
String scheduleServiceBaseUrl = ConfigReader.readConfigName("scheduleServiceBaseUrl");
return new ScheduleService(scheduleServiceBaseUrl);
}
private Slot createAppointment(Slot s, Doctor d, Patient p) {
String appointmentServiceBaseUrl = ConfigReader.readConfigName("appointmentServiceBaseUrl");
AppointmentService appointmentService = new AppointmentService(appointmentServiceBaseUrl);
s.setIsOccupied(true);
return appointmentService.bookSlot(d, p, s);
}
private void notifyDoctor(Slot s, Doctor d, Patient p) {
DoctorEmailTemplate doctorEmailTemplate = new DoctorEmailTemplate(d, p, bookedSlot);
notificationService.notify(d, doctorEmailTemplate);
}
private void notifyPatient(Slot s, Doctor d, Patient p) {
PatientEmailTemplate patientEmailTemplate = new PatientEmailTemplate(d, p, bookedSlot);
notificationService.notify(p, patientEmailTemplate);
}
Note the buildScheduleService method. If I really want to understand how ScheduleService is invoked to get a doctor’s schedule, this still looks all right, although just about at this stage someone will start arguing that service creation should not be done in this class or that it can be more generically for all service. That’s fine, it can still stand alone as a method, if not in this class then elsewhere. But the question indicates the curiousity/astonishment quotient has started rising.
Let’s take it one step further (gist).
public Slot makeAppointment(Doctor d, Hospital h, Patient p, Date startTime, Date endTime) throws Exception {
if (!isHospitalOpen(h, startTime, endTime)) {
throwException("Hospital isn't open in this time period");
}
Slot s = getFreeSlotForDoctor(d, startTime, endTime);
if (s == null) {
throwException("Doctor is not free in this time period");
}
Slot bookedSlot = createAppointment(s, d, p);
notifyDoctor(bookedSlot, d, p);
notifyPatient(bookedSlot, d, p);
return bookedSlot;
}
private void throwException(String message) throws Exception{
throw new Exception(message);
}
private boolean isHospitalOpen(Hospital h, Date startTime, Date endTime) {
String hospitalServiceBaseUrl = ConfigReader.readConfigName("hospitalServiceBaseUrl");
HospitalService hospitalService = new HospitalService(hospitalServiceBaseUrl);
DateRange range = hospitalService.getWorkingHours();
return range.contains(startTime) && range.contains(endTime);
}
private boolean getFreeSlotForDoctor(Doctor d, Date startTime, Date endTime) {
return buildScheduleService.getAppointmentSlots(d).stream().filter(slot -> !slot.isOccupied) // Only consider free slots
.filter(slot -> slot.contains(startTime) && slot.contains(endTime)) // Only consider slots which contain this time period
.findFirst();
}
private ScheduleService buildScheduleService() {
String scheduleServiceBaseUrl = ConfigReader.readConfigName("scheduleServiceBaseUrl");
return new ScheduleService(scheduleServiceBaseUrl);
}
private Slot createAppointment(Slot s, Doctor d, Patient p) {
String appointmentServiceBaseUrl = ConfigReader.readConfigName("appointmentServiceBaseUrl");
AppointmentService appointmentService = new AppointmentService(appointmentServiceBaseUrl);
s.setIsOccupied(true);
return appointmentService.bookSlot(d, p, s);
}
private void notifyDoctor(Slot s, Doctor d, Patient p) {
DoctorEmailTemplate doctorEmailTemplate = new DoctorEmailTemplate(d, p, bookedSlot);
notificationService.notify(d, doctorEmailTemplate);
}
private void notifyPatient(Slot s, Doctor d, Patient p) {
PatientEmailTemplate patientEmailTemplate = new PatientEmailTemplate(d, p, bookedSlot);
notificationService.notify(p, patientEmailTemplate);
}
Any developer with any amount of experience in any codebase will get curious as to why we need a separate method just to throw an exception. They will try to go to the lower level to understand it, which is exactly what we set out to prevent. At this level of granularity, our method size is causing astonishment so we should abort this step.
The point at which engagement or curiousity begins to rise depends on the business and technical context of a codebase. So while this is obviously a simple example, preventing cognitive load is widely applicable as a guiding principle in software engineering. When reading a piece of code, keep in mind your engagement and curiousity levels. If either increase, there may be the possibility of refactoring.
From the internet this week
- Ingrid writes about why decentralised applications don't work in the real world. With crypto making news for all kinds of reasons, this is a thought-provoking take.
- Here's an old video of Brian Kerninghan interviewing Ken Thompson. The interaction between these two greats is worth listening to.
- Jamie Brandon discusses consistency in streaming systems.
- Our words shape our perspectives. Sarah Drasner reminds us that engineering managers should think of their team as "us", not "them".
That's it for this week folks!
Cheers!
Kislay |