Clean Code and Hybrid Objects and Feature Envy

Question

So I recently made some major refactorings to my code. One of the main things I tried to do was split out my classes into data objects and worker objects. This was inspired, among other things, by this section of Clean Code:

Hybrids

This confusion sometimes leads to unfortunate hybrid data structures that are half object and half data structure. They have functions that do significant things, and they also have either public variables or public accessors and mutators that, for all intents and purposes, make the private variables public, tempting other external functions to use those variables the way a procedural program would use a data structure.

Such hybrids make it hard to add new functions but also make it hard to add new data structures. They are the worst of both worlds. Avoid creating them. They are indicative of a muddled design whose authors are unsure of - or worse, ignorant of - whether they need protection from functions or types.

Recently I was looking at the code to one of my worker objects (that happens to implement the Visitor Pattern) and saw this:

@Override
public void visit(MarketTrade trade) {
    this.data.handleTrade(trade);
    updateRun(trade);
}

private void updateRun(MarketTrade newTrade) {
    if(this.data.getLastAggressor() != newTrade.getAggressor()) {
        this.data.setRunLength(0);
        this.data.setLastAggressor(newTrade.getAggressor());
    }
    this.data.setRunLength(this.data.getRunLength() + newTrade.getLots());
}

I immediately said to myself "feature envy! this logic should be in the Data class - specifically in the handleTrade method. handleTrade and updateRun should always happen together". But then I thought "the data class is just a public data structure, if I start doing that, then it will be come a Hybrid Object!"

What's better and why? How do you decide which to do?

Answer 1

The text you quoted has good advice, although I will replace “data structures” by “records”, assuming that something like structs is meant. Records are just dumb aggregations of data. While they might be mutable (and thus stateful in a functional-programming mindset), they don't have any internal state, no invariants that have to be protected. It is completely valid to add operations to a record that make its usage easier.

For example, we can argue that a 3D vector is a dumb record. However, that should not prevent us from adding a method like add, which makes adding vectors easier. Adding behaviour does not turn a (not so dumb) record into a hybrid.

This line is crossed when the public interface of an object allows us to break encapsulation: There are some internals which we can access directly, thus bringing the object into an invalid state. It seems to me that Data does have state, and that it can be brought into an invalid state:

• After handling a trade, the last aggressor might not be updated.
• The last aggressor can be updated even when no new trade occurred.
• The run length might retain its old value even when the aggressor was updated.
• etc.

If any state is valid for your data, then everything is fine with your code, and you can carry on. Otherwise: The Data class is responsible for its own data consistency. If handling a trade always involves updating the aggressor, this behaviour has to be part of the Data class. If changing the aggressor involves setting the run length to zero, this behaviour has to be part of the Data class. Data was never a dumb record. You already made it a hybrid by adding public setters.

There is one scenario in which you can consider relaxing these strict responsibilities: If Data is private to your project and is therefore not part of any public interface, you can still ensure proper usage of the class. However, this places the responsibility for maintaining Data consistency throughout the code, rather than collecting them at a central location.

I recently wrote an answer on encapsulation, which goes into more depth on what encapsulation is and how you can ensure it.

Answer 2

The fact that handleTrade() and updateRun() always happen together (and the second method is actually on the visitor and calls several other methods on the data object) smells of temporal coupling. This means that you must call methods in a specific order, and I would guess that calling methods out of order will break something at worst, or fail to provide a meaningful result at best. Not good.

Typically the correct way to refactor that dependency out is for each method to return a result which can either be fed into the next method or acted upon directly.

Old code:

MyObject x = ...;
x.actionOne();
x.actionTwo();
String result = x.actionThree();

New code:

MyObject x = ...;
OneResult r1 = x.actionOne();
TwoResult r2 = r1.actionTwo();
String result = r2.actionThree();

This has several advantages:

• It moves separate concerns into separate objects (SRP).
• It removes temporal coupling: it is impossible to call methods out of order, and the method signatures provide implicit documentation about how to call them. Have you ever looked at documentation, seen the object you want, and worked backwards? I want object Z. But I need a Y to get a Z. To get a Y, I need an X. Aha! I have a W, which is required to get an X. Chain it all together, and your W can now be used to get a Z.
• Splitting objects like this is more likely to make them immutable, which has numerous advantages beyond the scope of this question. The quick takeaway is that immutable objects tend to lead to code that is more safe.

Answer 3

From my point of view a class should contain "values for state (member variables) and implementations of behavior (member functions, methods)".

The "unfortunate hybrid data structures" emerges if you make class state member-variables (or their getters/setters) public that should not be public.

So I see not need to have seperate classes for data data objects and worker objects.

You should be able to keep state-member-variables non-public (your database-layer should be able to handle non-public member-variables)

A Feature envy is a class that uses methods of another class excessively. See Code_smell. Having a class with methods and state would eliminate this.