Functions
Functions should be small and do one thing.
The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that.
public static List<UserReportDto> getUserReportDtos() {
List<User> users = CsvUtils.read(new File("user.csv"), User.class);
List<UserReportDto> userReportDtos = new ArrayList<>();
if (users != null && !users.isEmpty()) {
for (User user : users) {
UserReportDto reportDto = UserReportDto.builder()
.withName(user.getName())
.withEmail(user.getEmail())
.build;
userReportDtos.add(reportDto);
}
}
return userReportDtos;
}
// First level of abstraction
public static List<UserReportDto> getUserReportDtos() {
List<User> users = readUsersFromCsvFile("user.csv");
return mapToUserReportDtos(users);
}
// Second level of abstraction
private List<User> readUsersFromCsvFile(String csvFileName) {
return CsvUtils.read(new File(csvFileName), User.class);
}
// Second level of abstraction
private List<UserReportDto> mapToUserReportDtos(List<User> users) {
List<UserReportDto> userReportDtos = new ArrayList<>();
if (!isValid(users)) {
return userReportDtos;
}
for (User user : users) {
UserReportDto reportDto = mapToUserReportDto(user);
userReportDtos.add(reportDto);
}
return userReportDtos;
}
// Third level of abstraction
private boolean isValid(List<User> users) {
return users != null && !users.isEmpty();
}
// Third level of abstraction
private UserReportDto mapToUserReportDto(User user) {
return UserReportDto.builder()
.withName(user.getName())
.withEmail(user.getEmail())
.build();
}
One way to know that a function is doing more than “one thing” is if you can extract another function from it with a name that is not merely a restatement of its implementation.
In order to make sure our functions are doing “one thing,” we need to make sure that the statements within our function are all at the same level of abstraction.
Abstraction is a fundamental concept in OOPS. It talks about hiding the “how” part and only exposing “what” to the outer world.
According to SLAP (Single Level of Abstraction Principle), source code inside each method should refer to concepts and mechanisms relevant to just one level of “operational complexity” of your application.
Apply refactoring technique "Extract Method".
We want every function to be followed by those at the next level of abstraction so that we can read code from top to bottom.
public boolean isUserValid(UserDto userDto) {
if (userDto != null) {
if (userDto.getPassword() != null) {
if (userDto.getEmail() != null) {
return !userService.isEmailExists(userDto.getEmail());
}
}
}
return false;
}
public boolean isUserValid(UserDto userDto) {
if (userDto == null) {
return false;
}
if (userDto.getPassword() == null) {
return false;
}
if (userDto.getEmail() == null) {
return false;
}
return !userService.isEmailExists(userDto.getEmail());
}
Negatives are just a bit harder to understand than positives. So, when possible, conditionals should be expressed as positives.
if(!isNotEmpty(users)) {}
if(isEmpty(users)) {}
It’s hard to make a small
switch statement. By their nature, switch statements always do N things. Unfortunately, we can’t always avoid switch statements, but we can make sure that each switch statement is buried in a low-level class and is never repeated. We do this, of course, with polymorphism.There are several problems with the switch statement.
- First, it’s large, and when new enum types are added, it will grow.
- Second, it very clearly does more than one thing.
- Third, it violates the Single Responsibility Principle (SRP) because there is more than one reason for it to change.
- Fourth, it violates the Open Closed Principle (OCP) because it must change whenever new types are added.
enum VehicleType {
BICYCLE,
CAR,
AIRPLANE
}
class Vehicle {
String name;
VehicleType type;
void ride() {}
void fly() {}
}
void perform(Vehicle vehicle) {
switch (vehicleType) {
case BICYCLE:
vehicle.ride();
break;
case CAR:
vehicle.drive();
break;
case AIRPLANE:
vehicle.fly();
break;
default:
break;
}
}
interface VehicleAction {
void perform();
}
abstract class Vehicle implements VehicleAction {
String name;
}
class Bicycle extends Vehicle {
void ride() {}
@Override
public void perform() {
ride();
}
}
class Car extends Vehicle {
void drive() {}
@Override
public void perform()
drive();
}
}
// App
void perform(VehicleAction vehicleAction) {
vehicleAction.perform();
}
Don’t be afraid to make a name long. A long descriptive name is better than a short enigmatic name. A long descriptive name is better than a long descriptive comment.
public void createPrd(); // Create a product
public void createProduct();
Be consistent in your names. Use the same phrases, nouns, and verbs in the function names you choose for your modules.
beforeEach();
afterEach();
beforeAll();
afterAll();
The ideal number of arguments for a function is zero (niladic). Next comes one (monadic), followed closely by two (dyadic). Three arguments (triadic) should be avoided where possible. More than three (polyadic) requires very special justification—and then shouldn’t be used anyway.
void createUser(String username, String password, Gender gender, int roleId);
Apply refactoring technique "Replace Method Parameters with Method Object": When a function seems to need more than two or three arguments, it is likely that some of those arguments ought to be wrapped into a class of their own.
void createUser(UserDto userDto) {
}
class UserDto {
private String username;
private String password;
private Gender gender;
private int roleId;
}
Functions that take variable arguments can be monads, dyads, or even triads. But it would be a mistake to give them more arguments than that.
Arguments are even harder from a testing point of view. Imagine the difficulty of writing all the test cases to ensure that all the various combinations of arguments work properly.
Sometimes we want to pass a variable number of arguments into a function. Consider to use Argument Lists
public String format(String format, Object… args);
String.format(”%s worked %.2f hours.”, name, hours);
Function with flag argument does more than one thing. It does one thing if the flag is true and another if the flag is false!
public execute(boolean isTestSuite) {
if (isSuite) {
// do something for suite
} else {
// do something for single test
}
}
public void executeForTestSuite() {
// do something for suite
}
public void executeForSingleTest() {
// do something for single test
}
Split the method into several independent methods that can be called from the client without the flag.
Side effects are lies. Your function promises to do one thing, but it also does other hidden things.
The side effect is the call to update(). The get user function, by its name, says that it gets the user by email. The name does not imply that it updates the last access of the user.
public User getUserByEmail(String email) {
User user = userRepository.getUserByEmail(email);
user.setLastAccess(now());
userRepository.update(user);
return user;
}
Anything that gets modified or mutated but is not returned by the function. An array/object passed to a function, which changes the body of the array or changes the data of the object but does not return it, then it is the output argument.
public User createUser(UserDto userDto) {
userDto.setRole("ADMIN");
return userRepository.createUser(userDto);
}
class UserDto {
private String email;
private String password;
private String role;
}
Output arguments should be avoided. If your function must change the state of something, have it change the state of its owning object.
public void appendFooter(StringBuffer report);
appendFooter(report);
report.appendFooter();
Functions should either do something or answer something, but not both.
public boolean set(String attribute, String value);
if (set("username", "gpcoder")) {}
if (attributeExists("username")) {
setAttribute("username", "gpcoder");
}
Returning error codes from command functions lead to deeply nested structures and maybe a violation of command query separation.
public void sendShutDown() {
DeviceHandle handle = getHandle(DEV1);
// Check the state of the device
if (handle != DeviceHandle.INVALID) {
// Save the device status to the record field
retrieveDeviceRecord(handle);
// If not suspended, shut down
if (record.getStatus() != DEVICE_SUSPENDED) {
pauseDevice(handle);
clearDeviceWorkQueue(handle);
closeDevice(handle);
} else {
logger.log("Device suspended. Unable to shut down");
}
} else {
logger.log("Invalid handle for: " + DEV1.toString());
}
}
public void sendShutDown() {
try {
tryToShutDown();
} catch (DeviceShutDownError e) {
logger.log(e);
}
}
private void tryToShutDown() throws DeviceShutDownError {
DeviceHandle handle = getHandle(DEV1);
DeviceRecord record = retrieveDeviceRecord(handle);
pauseDevice(handle);
clearDeviceWorkQueue(handle);
closeDevice(handle);
}
private DeviceHandle getHandle(DeviceID deviceID) {
throw new DeviceShutDownError("Invalid handle for: " + deviceID);
}
Another exmaple:
public void performLogin(UserDto userDto) {
ResponseCode status = login(userDto);
switch (status) {
case BAD_REQUEST:
log.error("Bad request");
break;
case USER_NOT_FOUND:
log.error("Invalid username or password");
break;
case WRONG_PASSWORD:
log.error("Invalid username or password");
break;
case SUCCESS:
log.error("Login success");
redirect();
break;
default:
log.error("Unknown response");
break;
}
}
public ResponseCode login(UserDto userDto) {
if (userDto.getEmail() == null || userDto.getPassword() == null) {
return ResponseCode.BAD_REQUEST;
}
User user = userRepository.get(userDto);
if (user == null) {
return ResponseCode.USER_NOT_FOUND;
}
if (notMatchPassword(user.getPassword, userDto.getPassword())) {
return ResponseCode.WRONG_PASSWORD;
}
setUserSession(user);
return SUCCESS;
}
public void performLogin(UserDto userDto) {
try {
login(userDto);
} catch(Exception ex) {
log.error(ex.getMessage(), ex);
}
redirect();
}
public void login(UserDto userDto) {
if (userDto.getEmail() == null || userDto.getPassword() == null) {
throw new BadRequestException("Bad request");
}
User user = userRepository.get(userDto);
if (user == null) {
throw new UserNotFoundException("Invalid username or password");
}
if (notMatchPassword(user.getPassword, userDto.getPassword())) {
throw new UserNotFoundException("Invalid username or password");
}
setUserSession(user);
}
When you use exceptions rather than error codes, then new exceptions are derivatives of the exception class. They can be added without forcing any recompilation or redeployment.
It is better to extract the bodies of the
try and catch blocks out into functions of their own.If the keyword try exists in a function, it should be the very first word in the function and there should be nothing after the catch/finally blocks.
public void performLogin(UserDto userDto) {
try {
login(userDto);
} catch(Exception ex) {
log.error(ex.getMessage(), ex);
throw new BadCredentialExceptoin();
}
redirect(); // Should move to try block
}
public void performLogin(UserDto userDto) {
try {
login(userDto);
redirect();
} catch(Exception ex) {
log.error(ex.getMessage(), ex);
throw new BadCredentialExceptoin();
}
}
Every block within a function should have one entry and one exit. Following these rules means that there should only be one
return statement in a function, no break or continue statements in a loop, and never, ever, any goto statements.If you keep your functions small, then the occasional multiple
return, break, or continue the statement does no harm and can sometimes even be more expressive than the single-entry, single-exit rule. On the other hand, goto only makes sense in large functions, so it should be avoided.
When I write functions, they come out long and complicated. But I also have unit tests that cover every one of those clumsy lines of code. So then I massage and refine that code, splitting out functions, changing names, eliminating duplication.— Robert C. Martin