Published by Marco on 3. Jun 2016 13:09:03
------------------------------------------------------------------------

[Overview]

We discussed ABD in a recent article "ABD: Refactoring and refining an API"
. To cite from that article,

"[...] the most important part of code is to think about how you’re writing it
and what you’re building. You shouldn’t write a single line without thinking of
the myriad ways in which it must fit into existing code and the established
patterns and practices."

With that in mind, I saw another teaching opportunity this week and wrote up my
experience designing an improvement to an existing API.

[Requirements]

Before we write any code, we should know what we're doing. [1]

  * We use aspects (IMetaAspects) in Quino to add domain-specific metadata (e.g.
    the IVisibleAspect controls element visibility)
  * Suppose we have such an aspect with properties A1...AN. When we set property
    A1 to a new value, we want to retain the values of properties A2...AN (i.e.
    we don't want to discard previously set values)
  * The current pattern is to call FindOrAddAspect(). This method does what it
    advertises: If an aspect with the requested type already exists, it is
    returned; otherwise, an instance of that type is created, added and
    returned. The caller gets an instance of the requested type (e.g.
    IVisibleAspect).
  * Any properties on the requested type that you want to change must have
    setters.
  * If the requested type is an interface, then we end up defining our interface
    as mutable.
  * Other than when building the metadata, every other use of these interfaces
    should not make changes.
  * We would like to be able to define the interface as read-only (no setters)
    and make the implementation mutable (has setters). Code that builds the
    metadata uses both the interface and the implementation type.

Although we're dealing concretely with aspects in Quino metadata, the pattern
and techniques outlined below apply equally well to other, similar domains.

[The current API]

A good example is the IClassCacheAspect. It exposes five properties, four of
which are read-only. You can modify the property (OrderOfMagnitude) through the
interface. This is already not good, as we are forced to work with the
implementation type in order to change any property other than OrderOfMagnitude.

The current way to address this issue would be to make all of the properties
settable on the interface. Then we could use the FindOrAddAspect() method with
the IClassCacheAspect. For example,


var cacheAspect = 
  Element.Classes.Person.FindOrAddAspect(
    () => new ClassCacheAspect()
  );
cacheAspect.OrderOfMagnitude = 7;
cacheAspect.Capacity = 1000;

For comparison, if the caller were simply creating the aspect instead of getting
a possibly-already-existing version, then it would just use an object
initializer.


var cacheAspect = Element.Classes.Person.Aspects.Add(
  new ClassCacheAspect()
  {
    OrderOfMagnitude = 7;
    Capacity = 1000;
  }
}

This works nicely for creating the initial aspect. But it causes an error if an
aspect of that type had already been added. Can we design a single method with
all the advantages?

[The new API]

A good way to approach a new is to ask: How would we want the method to look if
we were calling it?


Element.Classes.Person.SetCacheAspectValues(
  a =>
  {
    a.OrderOfMagnitude = 7;
    a.Capacity = 1000;
  }
);

If we only want to change a single property, we can use a one-liner:


Element.Classes.Person.SetCacheAspectValues(a => a.Capacity = 1000);

Nice. That's even cleaner and has fewer explicit dependencies than creating the
aspect ourselves.

[Making it work for one aspect type]

Now that we know what we want the API to look like, let's see if it's possible
to provide it. We request an interface from the list of aspects but want to use
an implementation to set properties. The caller has to indicate how to create
the instance if it doesn't already exist, but what if it does exist? We can't
just upcast it because there is no guarantee that the existing aspect is the
same implementation.

These are relatively lightweight objects and the requirement above is that the
property values on the existing aspect are set on the returned aspect, not that
the existing aspect is preserved.

What if we just provided a mechanism for copying properties from an existing
aspect onto the new version?


var cacheAspect = new ClassCacheAspect();
var existingCacheAspect =
  Element.Classes.Person.Aspects.FirstOfTypeOrDefault();
if (existingCacheAspect != null)
{
  result.OrderOfMagnitude = existingAspect.OrderOfMagnitude;
  result.Capacity = existingAspect.Capacity;
  // Set all other properties
}

// Set custom values
cacheAspect.OrderOfMagnitude = 7;
cacheAspect.Capacity = 1000;

This code does exactly what we want and doesn't require any setters on the
interface properties. Let's pack this away into the API we defined above. The
extension method is:


public static ClassCacheAspect SetCacheAspectValues(
  this IMetaClass metaClass,
  Action setValues)
{
  var result = new ClassCacheAspect();
  var existingCacheAspect =
    metaClass.Aspects.FirstOfTypeOrDefault();
  if (existingCacheAspect != null)
  {
    result.OrderOfMagnitude = existingAspect.OrderOfMagnitude;
    result.Capacity = existingAspect.Capacity;
    // Set all other properties
  }

  setValues(result);

  return result;
}

So that takes care of the boilerplate for the IClassCacheAspect. It hard-codes
the implementation to ClassCacheAspect, but let's see how big a restriction that
is once we've generalized below.

[Generalize the aspect type]

We want to see if we can do anything about generalizing SetCacheAspectValues()
to work for other aspects.

Let's first extract the main body of logic and generalize the aspects.


public static TConcrete SetAspectValues(
  this IMetaClass metaClass,
  Action copyValues,
  Action setValues
)
  where TConcrete : new, TService
  where TService : IMetaAspect
{
  var result = new TConcrete();
  var existingAspect = metaClass.Aspects.FirstOfTypeOrDefault();
  if (existingAspect != null)
  {
    copyValues(result, existingAspect);
  }

  setValues(result);

  return result;
}

[Remove constructor restriction]

This isn't bad, but we've required that the TConcrete parameter implement a
default constructor. Instead, we could require an additional parameter for
creating the new aspect.


public static TConcrete SetAspectValues(
  this IMetaClass metaClass,
  Func createAspect,
  Action copyValues,
  Action setValues
)
  where TConcrete : TService
  where TService : IMetaAspect
{
  var result = createAspect();
  var existingAspect = metaClass.Aspects.FirstOfTypeOrDefault();
  if (existingAspect != null)
  {
    copyValues(result, existingAspect);
  }

  setValues(result);

  return result;
}

[Just pass in the new aspect to use]

Wait, wait, wait. We not only don't need to the new generic constraint, we also
don't need the createAspect lambda parameter, do we? Can't we just pass in the
object instead of passing in a lambda to create the object and then calling it
immediately?


public static TConcrete SetAspectValues(
  this IMetaClass metaClass,
  TConcrete aspect,
  Action copyValues,
  Action setValues
)
  where TConcrete : TService
  where TService : IMetaAspect
{
  var existingAspect = metaClass.Aspects.FirstOfTypeOrDefault();
  if (existingAspect != null)
  {
    copyValues(aspect, existingAspect);
  }

  setValues(aspect);

  return aspect;
}

That's a bit more logical and intuitive, I think.

[Redefine original method]

We can now redefine our original method in terms of this one:


public static ClassCacheAspect SetAspectValues(
  this IMetaClass metaClass,
  Action setValues)
{
  return metaClass.UpdateAspect(
    new ClassCacheAspect(),
    (aspect, existingAspect) =>
    {
      result.OrderOfMagnitude = existingAspect.OrderOfMagnitude;
      result.Capacity = existingAspect.Capacity;
      // Set all other properties
    },
    setValues
  );
}

[Generalize copying values]

Can we somehow generalize the copying behavior? We could make a wrapper that
expects an interface on the TService that would allow us to call
CopyFrom(existingAspect).


public static TConcrete SetAspectValues(
  this IMetaClass metaClass,
  TConcrete aspect,
  Action setValues
)
  where TConcrete : TService, ICopyTarget
  where TService : IMetaAspect
{
  return metaClass.UpdateAspect(
    aspect,
    (aspect, existingAspect) => aspect.CopyFrom(existingAspect),
    setValues
  );
}

What does the ICopyTarget interface look like?


public interface ICopyTarget
{
  void CopyFrom(object other);
}

This is going to lead to type-casting code at the start of every implementation
to make sure that the other object is the right type. We can avoid that by using
a generic type parameter instead.


public interface ICopyTarget
{
  void CopyFrom(T other);
}

That's better. How would we use it? Here's the definition for ClassCacheAspect:


public class ClassCacheAspect : IClassCacheAspect,
ICopyTarget
{
  public void CopyFrom(IClassCacheAspect otherAspect)
  {
    OrderOfMagnitude = otherAspect.OrderOfMagnitude;
    Capacity = otherAspect.Capacity;
    // Set all other properties
  }
}

Since the final version of ICopyTarget has a generic type parameter, we need to
adjust the extension method. But that's not a problem because we already have
the required generic type parameter in the outer method.


public static TConcrete SetAspectValues(
  this IMetaClass metaClass,
  TConcrete aspect,
  Action setValues
)
  where TConcrete : TService, ICopyTarget
  where TService : IMetaAspect
{
  return metaClass.UpdateAspect(
    aspect,
    (aspect, existingAspect) => aspect.CopyFrom(existingAspect),
    setValues
  );
}

[Final implementation]

Assuming that the implementation of ClassCacheAspect implements ICopyTarget as
shown above, then we can rewrite the cache-specific extension method to use the
new extension method for ICopyTargets.


public static ClassCacheAspect SetCacheAspectValues(
  this IMetaClass metaClass,
  Action setValues)
{
  return metaClass.UpdateAspect(
    new ClassCacheAspect(),
    setValues
  );
}

This is an extension method, so any caller that wants to use its own
IClassCacheAspect could just copy/paste this one line of code and use its own
aspect.

[Conclusion]

This is actually pretty neat and clean:

  * We have a pattern where all properties on the interface are read-only
  * We have a pattern where an aspect can indicate how its values are to be
    copied from another instance. This is basically boilerplate, but must be
    written only once per aspect -- and it can be located right in the
    implementation itself rather than in an extension method.
  * A caller building metadata passes in a single lambda to set values. Existing
    values are handled automatically.
  * Adding support for more aspects is straightforward and involves very little
    boilerplate.

--------------------------------------------------------------------------------


[1] You would think that would be axiomatic. You'd be surprised.