Property initialisation anti-pattern

Question

Now and again I end up with code along these lines, where I create some objects then loop through them to initialise some properties using another class...

ThingRepository thingRepos      = new ThingRepository();
GizmoProcessor  gizmoProcessor  = new GizmoProcessor();
WidgetProcessor widgetProcessor = new WidgetProcessor();

public List<Thing> GetThings(DateTime date)
{
    List<Thing> allThings = thingRepos.FetchThings();

    // Loops through setting thing.Gizmo to a new Gizmo
    gizmoProcessor.AddGizmosToThings(allThings);

    // Loops through setting thing.Widget to a new Widget
    widgetProcessor.AddWidgetsToThings(allThings);

    return allThings;
}

...which just, well, feels wrong.

1. Is this a bad idea?
2. Is there a name of an anti-pattern that I'm using here?
3. What are the alternatives?

---

Edit: assume that both GizmoProcessor and WidgetProcessor have to go off and do some calculation, and get some extra data from other tables. They're not just data stored in a repository. They're creating new Gizmos and Widgets based on each Thing and assigning them to Thing's properties.

The reason this feels odd to me is that Thing isn't an autonomous object; it can't create itself and child objects. It's requiring higher-up code to create a fully finished object. I'm not sure if that's a bad thing or not!

Answer 1

If you have complex intialization then you could use a Strategy pattern. Here is a quick overview adapted from this strategy pattern overview

Create a strategy interface to abstract the intialization

public interface IThingInitializationStrategy
{
  void Initialize(Thing thing);
} 

The initialization implementation that can be used by the strategy

public class GizmosInitialization
{
  public void Initialize(Thing thing)
  {
     // Add gizmos here and other initialization
  }
}

public class WidgetsInitialization
{
  public void Initialize(Thing thing)
  {
    // Add widgets here and other initialization
  }
}

And finally a service class that accepts the strategy implementation in an abstract way

internal class ThingInitalizationService
{
  private readonly IThingInitializationStrategy _initStrategy;

  public ThingInitalizationService(IThingInitializationStrategy initStrategy)
  {
     _initStrategy = initStrategy;
  }

  public Initialize(Thing thing)
  {
    _initStrategy.Initialize(thing);
  }
}

You can then use the initialization strategies like so

var initializationStrategy = new GizmosInitializtion();
var initializationService = new ThingInitalizationService(initializationStrategy);


List<Thing> allThings = thingRepos.FetchThings();

allThings.Foreach ( thing => initializationService.Initialize(thing) );

Answer 2

ThingRepository is supposed to be the single access point to get collections of Thing's, or at least that's where developers will intuitively look. For that reason, it feels strange that GetThings(DateTime date) should be provided by another object. I'd rather place that method in ThingRepository itself.

The fact that the Thing's returned by GetThings(DateTime date) are different, "fatter" animals than those returned by ThingRepository.FetchThings() also feels awkward and counter-intuitive. If Gizmo and Widget are really part of the Thing entity, you should be able to access them every time you have an instance of Thing, not just for instances returned by GetThings(DateTime date).

If the Date parameter in GetThings() isn't important or could be gathered at another time, I would use calculated properties on Thing to implement on-demand access to Gizmo and Widget :

public class Thing
{
  //...

  public Gizmo Gizmo
  {
    get 
    { 
       // calculations here 
    }
  }

  public Widget Widget
  {
    get 
    { 
       // calculations here 
    }
  }
}

Note that this approach is valid as long as the calculations performed are not too costly. Calculated properties with expensive processing are not recommended - see http://msdn.microsoft.com/en-us/library/bzwdh01d%28VS.71%29.aspx#cpconpropertyusageguidelinesanchor1

However, these calculations don't have to be implemented inline in the getters - they can be delegated to third-party Gizmo/Widget processors, potentially with a caching strategy, etc.

Answer 3

Tho only real potential problem would be that you're iterating over the same loop multiple times, but if you need to hit a database to get all the gizmos and widgets then it might be more efficient to request them in batches so passing the full list to your Add... methods would make sense.

The other option would be to look into returning the gizmos and widgets with the thing in the first repository call (assuming they reside in the same repo). It might make the query more complex, but it would probably be more efficient. Unless of course you don't ALWAYS need to get gizmos and widgets when you fetch things.

Answer 4

To answer your questions:

1. Is this a bad idea?

   • From my experience, you rarely know if it's a good/bad idea until you need to change it.
   • IMO, code is either: Over-engineered, under-engineered, or unreadable
   • In the meantime, you do your best and stick to the best practices (KISS, single responsibility, etc)
   • Personally, I don't think the processor classes should be modifying the state of any Thing.
   • I also don't think the processor classes should be given a collection of Things to modify.

2. Is there a name of an anti-pattern that I'm using here?

   • Sorry, unable to help.

3. What are the alternatives?

Personally, I would write the code as such:

public List<Thing> GetThings(DateTime date)
{
    List<Thing> allThings = thingRepos.FetchThings();

    // Build the gizmo and widget for each thing
    foreach (var thing in allThings)
    {
        thing.Gizmo = gizmoProcessor.BuildGizmo(thing);
        thing.Widget = widgetProcessor.BuildWidget(thing);
    }

    return allThings;
}

My reasons being:

1. The code is in a class that "Gets things". So logically, I think it's acceptable for it to traverse each Thing object and initialise them.
2. The intention is clear: I'm initialising the properties for each Thing before returning them.
3. I prefer initialising any properties of Thing in a central location.
4. I don't think that gizmoProcessor and widgetProcessor classes should have any business with a Collection of Things
5. I prefer the Processors to have a method to build and return a single widget/gizmo

However, if your processor classes are building several properties at once, then only would I refactor the property initialisation to each processor.

public List<Thing> GetThings(DateTime date)
{
    List<Thing> allThings = thingRepos.FetchThings();

    // Build the gizmo and widget for each thing
    foreach (var thing in allThings)
    {
        // [Edited]
        // Notice a trend here: The common Initialize(Thing) interface
        // Could probably be refactored into some 
        // super-mega-complex Composite Builder-esque class should you ever want to
        gizmoProcessor.Initialize(thing);
        widgetProcessor.Initialize(thing);
    }

    return allThings;
}

P.s.:

• I personally do not care that much for (Anti)Pattern names.
• While it helps to discuss a problem at a higher level of abstraction, I wouldn't commit every (anti)pattern names to memory.
• When I come across a Pattern that I believe is helpful, then only do I remember it.
• I'm quite lazy, and my rationale is that: Why bother remembering every pattern and anti pattern if I'm only going to use a handful?

[Edit]

Noticed an answer was already given regarding using a Strategy Service.