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?

  • 2
    Why does 'data' have to be a data structure at all. It has clear behaviour. So just turn off all the getters and setters, so that no object can manipulate internal state. – Cormac Mulhall Oct 13 '14 at 9:14
up vote 9 down vote accepted
+50

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.

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.
  • There is no temporal coupling between those two method calls. Swap their order and the behavior does not change. – durron597 Oct 13 '14 at 14:48
  • 1
    I initially also thought of sequential/temporal coupling when reading the question, but then noticed that the updateRun method was private. Avoiding sequential coupling is good advice, but it only applies to API design/public interfaces, and not to implementation details. The real question seems to be whether updateRun should be in the visitor or in the data class, and I don't see how this answer addresses that problem. – amon Oct 13 '14 at 15:21
  • The visibility of updateRun is irrelevant, what is important is the implementation of this.data which is not present in the question and is the object being manipulated by the visitor object. – user22815 Oct 13 '14 at 15:50
  • If anything, the fact that this visitor is simply calling a bunch of setters and not actually processing anything is a reason for temporal coupling not being present. It likely does not matter what order the setters are being called. – user22815 Oct 13 '14 at 16:01

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.

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.