A design discussion, some options, and unidirectional models

When writing about discoverability in my last two posts, I came across the below code which violated that principle.  It was doing some meta programming in order to attempt to avoid repetition and to be more DRY.

def base_value_for(key)
    case info.send("#{key}_base")
    when 'value1'
        value1
    when 'value2'
        value2
    else
        raise ArgumentError, "Base keys must be value1 or value2"
    end
end

The caller then would be able to find the 3 base values by passing them into the base_value_for method.  The methods one_base, two_base, three_base are known and exist on the info object which is receiving the public send.  The UI has ensured they are set to either ‘value1’ or ‘value2’.  The current object has a method to return the value for value1 and the value for value2.  So this code is essentially allowing the initial base value for a calculation to be configured.

The caller would then call one of base_value_for(‘one’) OR base_value_for(‘two’) OR base_value_for(‘three’).  Calling with any other value that didn’t result in a method on the info object for key_<value> would blow up.

What is wrong with this?  Take a read here.  TL;DR – if you’re in the info class and search for ‘one_base’ in the code base there will be no search hit that tells you that this code is being called from here.  So you may assume it isn’t being called and refactor it.  The principle of discoverability is violated.

This code was written to avoid writing:

def base_for_one_base
    case info.one_base
    when 'value1'
        value1
    when 'value2'
        value2
    else
        raise ArgumentError, "Base keys must be value1 or value2"
    end
end

and repeat for two and three.

An alternative to the above non-DRY implementation is

def base_for_one_base
    base_for(info.one_base)
end

private
def base_for(info_base)
    case info_base
    when 'value1'
        value1
    when 'value2'
        value2
    else
        raise ArgumentError, "Base keys must be value1 or value2"
    end
end

Now the difference between base_for_one_base and the version for two and three is just a single line.

def base_for_two_base
    base_for(info.two_base)
end

def base_for_three_base
    base_for(info.three_base)
end

This is probably DRY enough.  We could probably stop here.

An alternate design

The existence of the info model is to hold the knowledge about the configuration.  So an alternative design could be to push the decision logic into the info model.

The problem with doing this is that the method is also using the value1 and value2 methods on the parent model.

The one solution is to Just Use It.

# info class
belongs_to: parent

def base_for_one_base
    base_for(one_base)
end

private

def base_for(info_base)
    case info_base
    when value1
        parent.value1
    when 'value2'
        parent.value2
    else
        raise ArgumentError, "Base keys must be value1 or value2"
    end
end

This sets up a cyclic dependency.  The Parent model has an info model.  The Info belongs to the parent model.  The info can be loaded and accessed by its parent directly.  The parent can be loaded and accessed by the info model directly.  But what if code were written that caused code to be accidentally called recursively between the two models?

On this small scale, it might be okay.  On a larger scale it might be much harder to reason about across several models.  And multiple models may be doing this in a highly interconnected way – and you then may have one large ball of interconnected models – or mud.

Unidirectional models as a design constraint

What would the code look like if we didn’t accept cyclic dependencies?  If all interactions with a model were always from a parent to a child.  That could make the code simpler in the long run as at least half the number of linkages would exist.  One for each direction vs. one for the one direction.

The code in this example would then look like:

# parent class
def base_for_one_base
    info.base_for_one_base(value1, value2)
end

# info class
def base_for_one_base(value1, value2)
    base_for(one_base, value1, value2)
end

private

def base_for(info_base, value1, value2)
    case info_base
    when 'value1'
        value1
    when 'value2'
        value2
    else
        raise ArgumentError, "Base keys must be value1 or value2"
    end
end

The negative here is we need to pass in the data from the parent that the child needs.  Perhaps that data shouldn’t live on the parent, but rather move to the child.  That might be a better design and our code is telling us that, but it was something we didn’t want to move yet.  However that knowledge is not exposed to the consumer.  The implementation of getting the rate for the consumer is only via the parent.

Simplicity

I favour one-way connections over bi-directional ones as they are simpler to reason about.  They result in cleaner, contained messes that are easier to understand (and potentially replace).

This contrasts against the effort required to break down the keen desire to have bi-directional connections that Rails encourages and makes so easy.  SQL data integrity also desires bidirectional connections.

It might result in jumping through some hoops to achieve.  But then the design is speaking and the constraint is possibly telling me that the design of the data might need to change.

I favour smaller, contained messes to make reasoning and understanding of the code easier.  One way to achieve that is through unidirectional models.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s