A classic case of Data Envy

TL;TD: Refactoring classes that have data envy is easy. Do it.

Here is a snippet extracted from the last project I worked on:

// somewhere in a class
asset.setHasAssociatedImage(!StringUtils.isBlankOrNull(imageName));
asset.setImageName(imageName);

// in another class
if (asset.hasAssociatedImage()) {
	image.setUrl(categoryName + "/" + asset.getImageName()));
} else {
	image.setUrl(categoryName + "/" + "error.gif"));
}

If you’re like me, you’ll cringe at the sight. In fact, this is a classic case of Data Envy.


Quite obviously, simply by reading the calling code above, the implementation of the setHasAssociatedImage()/hasAssociatedImage() and setImageName()/getImageName() methods look like this:

public void setHasAssociatedImage(boolean hasAssociatedImage) {
	this.hasAssociatedImage = hasAssociatedImage;
}

public boolean hasAssociatedImage() {
	return hasAssociatedImage;
}

public void setImageName(String imageName) {
	this.imageName = imageName;
}

public String getImageName() {
	return imageName;
}

A typical Dumb Data Object.

To remove Date Envy, we can either move behavior to the record class, or pull data in the client class. When in doubt, I usually prefer to start by moving some behavior to it and replace the four methods above with the following two:

public void setImageName(String imageName) {
	this.imageName = imageName;
}

public String getImageName() {
	return StringUtils.isBlankOrNull(imageName) ? "error.gif" : imageName;
}

Which simplifies the client code to:

image.setUrl(categoryName + "/" + asset.getImageName()));

Not bad, except that it turns out that this client code is copied (copied & pasted, really) in two other classes, with variations in the name of the error image. We can refactor to:

public String getImageName(String errorImageName) {
	return StringUtils.isBlankOrNull(imageName) ? errorImageName : imageName;
}

and

image.setUrl(categoryName + "/" + asset.getImageName("error.gif")));

To go further, I much prefer when data classes are immutable. It makes it clear that their data can be read without fear of being changed in the middle of the processing (as a matter of fact, I like to make all my classes immutable, if I can). The data class then needs a constructor that takes in parameter all the data that the object may receive:

public class Record {
	private final String imageName;

	public Record(String imageName) {
		this.imageName = imageName;
	}

	public String getImageName(String errorImageName) {
		return StringUtils.isBlankOrNull(imageName) ? errorImageName : imageName;
	}
}