3
\$\begingroup\$

I'm looking to see if my current evolution of ETL logic is getting in the ball park of "how it's done in the real world".

  1. Specifically, do I use the metaphors correctly (Extract, Transform, Load)?
  2. Is this code reasonably clear for another developer's consumption?
  3. Is the OO nature clean or should it be decomposed more?

The steps are as follows:

  • Clear out the staging table
  • Get records from external system via service
  • Load records into staging table
  • Let a SQL view do the work of aggregating and transforming
  • Load rows into local database
class CakeSyncher
{
    public struct Parameters
    {
        public int OfferId;
        public int Year;
        public int Month;
        public int FromDay;
        public int ToDay;
        public DateTime FromDate { get { return new DateTime(this.Year, this.Month, this.FromDay); } }
        public DateTime ToDate { get { return new DateTime(this.Year, this.Month, this.ToDay); } }
    }

    private readonly Parameters parameters;
    private readonly ILogger logger;
    private readonly CakeWebService cakeService;
    private List<conversion> extractedConversions;
    private CakeEntities cakeEntities;
    private EomDatabaseEntities eomEntities;
    private List<CakeConversionSummary> conversionSummaries;
    private Dictionary<CakeConversionSummary, Item> itemsFromConversionSummaries;

    public CakeSyncher(ILogger logger, Parameters parameters)
    {
        this.parameters = parameters;
        this.logger = logger;
        this.cakeService = new CakeWebService(this.logger);
    }

    public void SynchStatsForOfferId()
    {
        try
        {
            using (this.cakeEntities = new CakeEntities())
            {
                this.DeleteExistingConversions();
                this.cakeEntities.SaveChanges();
            }
            using (this.cakeEntities = new CakeEntities())
            {
                this.ExtractConversions();
                this.StageExtractedConversions();
                this.cakeEntities.SaveChanges();
                using (this.eomEntities = EomDatabaseEntities.Create())
                {
                    this.ReadConversionSummaries();
                    this.TransformConversionSummariesToItems();
                    this.LoadItems();
                    this.eomEntities.SaveChanges();
                }
            }
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }

    #region Prepare
    private void DeleteExistingConversions()
    {
        this.logger.Log("Deleting existing conversions...");

        var existingConversions = this.cakeEntities.CakeConversions.ByOfferIdAndDateRange(this.parameters.OfferId,
                                                                                  this.parameters.FromDate,
                                                                                  this.parameters.ToDate);

        foreach (var conversion in existingConversions)
        {
            this.cakeEntities.CakeConversions.DeleteObject(conversion);
        }

        this.logger.Log("Deleted " + existingConversions.Count() + " conversions.");
    }
    #endregion

    #region Extract
    private void ExtractConversions()
    {
        logger.Log("Extracting conversions...");

        this.extractedConversions = this.cakeService.Conversions(this.parameters.OfferId,
                                                                 this.parameters.FromDate,
                                                                 this.parameters.ToDate);

        logger.Log("Extracted " + this.extractedConversions.Count + " conversions.");
    }

    private void StageExtractedConversions()
    {
        this.logger.Log("Staging extracted conversions...");

        this.extractedConversions.ForEach(extractedConversion =>
        {
            var cakeConversion = this.cakeEntities.CakeConversions.Create(extractedConversion.IdAsInt);
            cakeConversion.Update(extractedConversion);
        });

        this.logger.Log("Staged " + this.extractedConversions.Count + ".");
    }
    #endregion

    #region Transform
    private void ReadConversionSummaries()
    {
        this.conversionSummaries = cakeEntities.CakeConversionSummaries.ByOfferIdAndDateRange(this.parameters.OfferId,
                                                                                      this.parameters.FromDate,
                                                                                      this.parameters.ToDate).ToList();
    }

    private void TransformConversionSummariesToItems()
    {
        this.itemsFromConversionSummaries = this.conversionSummaries.ToDictionary(c => c, c => c.Item(this.eomEntities));
    } 
    #endregion

    #region Load
    private void LoadItems()
    {
        logger.Log("Loading items from conversion summaries...");

        foreach (var conversionSummary in this.itemsFromConversionSummaries.Keys.ToList())
        {
            var item = this.itemsFromConversionSummaries[conversionSummary];
            if (item == null)
            {
                item = CreateItem(conversionSummary, item);
            }
            else
            {
                logger.Log(string.Format("Updating item {0} from conversion {1}", item.name, conversionSummary.Name));
            }
            item.Update(this.eomEntities, conversionSummary, this.cakeService, this.logger);
        }

        logger.Log("Items loaded.");
    }

    private Item CreateItem(CakeConversionSummary conversionSummary, Item item)
    {
        logger.Log("Creating item: " + conversionSummary.Name);
        item = new Item();
        eomEntities.Items.AddObject(item);
        return item;
    }
    #endregion
}
\$\endgroup\$
2
\$\begingroup\$

A few things that caught my eye:

Naming

public Date FromDate {[...]}
public Date ToDate {[...]}

if that weren't readonly Properties, i'd assume they are conversion methods. Try to find a better name like: StartDate / EndDate.

Regions

Regions are cool. You can hide boilerplate code easily and structure large classes into nice small subsets. What you do here is something completely different. You Pack important Methods into very small logical structures. Why? Instead use the IDE to collapse the function to headers. This is just as efficient and definitely better than:

#region Load
private void LoadItems(){[...]}

private Item CreateItem(CakeConversionSummary conversionSummary, Item item){[...]}
#endregion

You hide CreateItem in a section called "Load". This makes no sense as creating is quite different from loading.

Var keyword

I really like the var-Keyword. IT seems you do too. But this creates a problem. There is no real gain in doing stuff like the following:

foreach(var conversionSummary in this.itemsFromConversionSummaries.Keys.ToList())
{
    var item = this.itemsFromConversionSummaries[conversionSummary];
    if (item == null)
    //...
}

When reading this code, I cannot help but ask myself: Why are you not using the Types?

foreach(ConversionSummary conversionSummary in this.itemsFromConversionSummaries.Keys.ToList())
{
    Item item = this.itemsFromConversionSummaries[conversionSummary];
    if (item == null)
    // ...
}

Type renaming is not a problem with the new IDE's.

Object Hierarchy

Code like this is smelly:

this.cakeEntities.CakeConversions.ByOfferIdAndDateRange([...]);

You are walking through the object hierarchy waay too long. This violates the Law of Demeter.

More formally, the Law of Demeter for functions requires that a method m of an object O may only invoke the methods of the following kinds of objects:

  • O itself
  • m's parameters
  • Any objects created/instantiated within m
  • O's direct component objects
  • A global variable, accessible by O, in the scope of m

Suggestion: move the ByOfferIdAndDateRange from the CakeConversions to the CakeEntities

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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