Class as Alternative to Parameter Threading
Instead of injecting values through method arguments and having to keep passing them wherever they are needed, consider creating an immutable object which encapsulates the logic and the needed values which are set on creation.
One benefit of this is improved readability. For instance, suppose we
have two sets, one of valid codes for something (validCodes
) and another of
allowed dates (allowedDates
). Compare:
if (isValidCode(validCodes, code)
&& isInAllowedDates(allowedDates, date)) {
/* do something */
}
vs.
if (isValidCode(code) && isInAllowedDate(date)) {
/* do something */
}
This can be useful when a method starts by getting a bunch of values which don’t change (say, getting a bunch of user IDs from the database for some kind of processing). Storing these on the enclosing class means setting them in a mutable (non-final) field, because these will be initialized in a method and not in the constructor. As a trimmed-down example, consider the following code which sends a coupon code:
public class CouponCodeEmailSendingJob implements Runnable {
private final UserService userService;
private final MailService mailService;
private Set<User> users;
private String emailTemplate;
public CouponCodeEmailSendingJob(UserService userService, MailService mailService) {
this.userService = userService;
this.mailService = mailService;
}
public void run() {
users = userService.getUsers();
emailTemplate = mailService.getCurrentEmailTemplate();
/* more code */
sendEmails();
/* mode code */
}
private void sendEmails() {
for (User user : users) {
mailService.sendCouponCodeEmail(user, emailTemplate);
}
}
}
For starters, the temporal dependency between assigning fields and using them isn’t explicit. In such small example it might be obvious, but the less I need to think and keep track off, the better. If we were dealing with a more complex case as it usually is, we might end up re-ordering statements so that we attempt to send emails before the users and emailTemplate fields are set. Presumably such glarious mistake would pop-up during testing, but it still would make us lose time.
We can make the dependency explicit by taking both as parameters for
sendEmails
, and making the variables local to the method and not fields.
final Set<User> users = userService.getUsers();
final String emailTemplate = mailService.getCurrentEmailTemplate();
/* more code */
sendEmails(users, emailTemplate);
/* mode code */
Now, if we reorder the code in an incorrect manner, it won’t compile. This is an improvement. But sometimes it’s not two parameters, but five, or more. And it’s not a single private method, but several, and we need to keep threading the parameters down to wherever they are needed. This can get unwidly fast.
Thread-safety is another issue with the original code that we’ve improved upon.
In the first iteration of the code, by having users
and emailTemplate
be
non-final instance variables, if the Runnable
would be called in parallel,
then we might change the users
and emailTemplate
fields before the first
thread attempts to send the emails (e.g. if the “current” email template is
another by the time the second thread runs). Because of that, making them
variables local to the method’s scope is safer.
But still the annoyance with threading parameters remains. In such cases, defining them as fields in a new class allows us to avoid this, and potentially move towards immutability, which would aid with making the code easier to understand, and closer to being thread-safe.
public class CouponCodeEmailSendingJob implements Runnable {
private final UserService userService;
private final MailService mailService;
public CouponCodeEmailSendingJob(UserService userService, MailService mailService) {
this.userService = userService;
this.mailService = mailService;
}
public void run() {
new InnerJob(userService.getPremiumUsers(), mailService.getCurrentEmailTemplate())
.run();
}
private class InnerJob implements Runnable {
private final Set<User> users;
private final String emailTemplate;
private InnerJob(Set<User> users, emailTemplate) {
this.users = users;
this.emailTemplate = emailTemplate;
}
public void run() {
/* more code ... */
sendEmails();
/* more code ... */
}
private void sendEmail() {
/ * ... * /
}
/* ... */
}
}
Now if we ever need to run this in parallel, each run has its own instance of
InnerJob
, so we don’t need to worry about race conditions. InnerJob
could
even have mutable state (say, keep track of statistics for logging how many
emails were sent, how many failed, etc.), and it wouldn’t be an issue because
InnerJob
is never shared between threads.
Of course we can still create implicit temporal couplings because the set of
users is shallowly immutable. A method could filter out some users from the set,
or add new ones, and then depending on when sendEmails()
is called, we would
send it to a set of users or another. Ideally though, we should treat the Set as
immutable and not modify it once it’s set. Any such filtering or additions
would be best performed before initializing InnerJob
with the Set
to use. If
so desired, we could enforce this by using some immutable set implementation
(e.g. by wrapping the Set
in Collections.unmodifiableSet
), but for small,
contained pieces of code, this is usually not needed.