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.

Bali - temple d'Ulu Watu
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;
	}
}

About Eric Lefevre-Ardant

Independent technical consultant.
This entry was posted in java. Bookmark the permalink.

4 Responses to A classic case of Data Envy

  1. Andrew Parker says:

    Absolutely love this! I think you can even take the whole thing a step further if you are using a Maybe or Option type. Change the signature of getImageName to

    Maybe getImageName()

    That then makes the call site read as

    asset.getImageName().otherwise("error.gif")

    This has the advantage of not assuming that the caller will always want to do a simple substitution of the name and preserves that the name may not always be available.

  2. Andrew Parker says:

    That should be Maybe<String>, but I didn’t escape the great than and less than symbols.

  3. Tom says:

    Little optimization: instead of doing the test in the getImageName() method, one can directly do it in the constructor. No?

  4. @andrew yes, you’re right, I do like your code better. I wonder how scary it is for people who haven’t seen that before?

    @tom yes, that does seem more efficient.

Leave a Reply

Your email address will not be published.

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>