I rewrote a very long method in which some data is queried from a database, based on info about a particular account, which is queried first.
I split out the account info into an immutable inner class AccountInfo
. This is a vastly simplified and altered version of what its constructor looks like:
private AccountInfo(String accountId, Optional<String> extraInfo) {
final List<Project> projects;
if (extraInfo.isPresent()) {
// (I could probably improve this and remove the .get() call)
projects = projectsDAO.fetchProjects(accountId, extraInfo.get());
if (projects.isEmpty()) {
this.projectIds = new ArrayList<>();
this.projectNames = new ArrayList<>();
this.unitIds = new ArrayList<>();
return;
}
} else {
projects = projectsDAO.fetchProjects(accountId);
}
projectIds = projects.stream().map(Project::getProjectId).collect(toList());
projectNames = projects.stream().map(Project::getProjectName).collect(toList());
final List<ProjUnit> units = unitsDAO.fetchUnitIds(projectIds);
this.projectIds = projectIds;
this.projectNames = projectNames;
this.unitsIds = units.stream().map(ProjUnit::getUnitId).limit(1000).collect(toList());
}
As you can see, the constructor queries the database multiple times, via the DAOs (in the real code it's four DAOs instead of two). Now, as far as I know, it's bad practice to have effectful computations inside a constructor (please correct me if that's wrong). There's four potential solutions that come to my mind:
1. Make the class mutable
If I didn't insist on doing everything inside the constructor, I could first create an empty instance, and then call some methods to populate it:
accountInfo = new AccountInfo(accountId, extraInfo);
accountInfo.fetchProjects()
accountInfo.fetchUnits()
Problems:
Immutable classes are generally preferred when possible, which seems especially relevant since this is a class that stores information that really shouldn't be modified once queried.
It cannot be guaranteed that the caller calls the
fetch
methods after calling the constructor.
2. Supply the queried data to the constructor
Instead of querying the database inside the constructor, the caller could supply the data to the constructor via parameters.
Problems:
This would remove some of the advantage gained from separating the class from the original method in the first place, since the point was to take this functionality and organize it as a unit.
The second query depends on results from the first, so either almost all of the logic would have to be done before the constructor is called, or I would need two separate immutable classes, one that gets the queried info about projects and a second one that gets that object and the queried info about units. (Possibly even more in the real code.)
3. Use static factory methods
Instead of inside the constructor, the querying could be done in a static factory method, leaving only the assignment to the actual fields to the constructor.
Problems:
Since static factory methods are very similar in responsibility to constructors, I'm not sure if it's any better for them to have side effects.
The DAO objects are part of the outer class instance, so a static method would have to receive these as parameters to be able to use them, which seems somewhat inelegant. Though it could perhaps be seen as an advantage, since it would make their use explicit.
4. Use something like a builder
This would be somewhat similar to the mutable option but seems strictly better, since it's safer. It would allow to do something like
AccountInfo accountInfo = AccountInfo.Builder(accountId, extraInfo, projectsDAO, unitsDAO).build()
And since the Builder itself would have the wrong type, it would ensure that the caller has to call build()
, which would handle all the side-effects.
Problems:
Similar to the static factory method option, it seems like it might be bad practice to have side effects in a
build
function (though perhaps this could be alleviated simply by renamingBuilder
andbuild
to something that doesn't evoke the same expectations)I think this also requires giving the Builder the DAOs as parameters (which, again, could potentially be seen as an advantage)
private AccountInfo(List<String> projectIDs, List<String> projectNames, List<Project> projects)
public static AccountInfo loadAccountFromDAO(ProjectsDAO projectsDAO, String accountId, Optional<String> extraInfo)