Loading and updating data from system A to system B

I need to transfer purchase order (PO) information from SQL Server into Sage 300 ERP, so using my Linq-to-Sage implementation, I wrote a little console application that connects to a SQL Server database to fetch PO header information, including some metadata telling me whether a purchase order...

• is new and should be created in Sage

• has been modified and should be updated in Sage, or

• was removed from the source system and needs to be deleted from Sage

The Program class looks like this:

class Program
{
    static void Main(string[] args)
    {
        var credentials = new SageCredential(SageSettings.Default.SageUserName, SageSettings.Default.SageUserPwd, SageSettings.Default.SageDb);
        using (var context = new SageContext(credentials))
        {
            context.Open();
            var app = new App(context, new PurchaseOrderLoader());
            app.Start();
        }
    }
}

The SageContext class implements a simple IUnitOfWork interface:

public interface IUnitOfWork
{
    void SaveChanges();
    IViewSet Set() where T : EntityBase;
}

The IPurchaseOrderLoader interface merely abstracts away some ADO.NET operations that fetch data from the SQL Server source, and update the metadata by marking a processed record as such:

public interface IPurchaseOrderLoader
{
    IEnumerable GetPurchaseOrderHeaders();
    void MarkProcessedPurchaseOrderHeader(int id);

    IEnumerable GetPurchaseOrderDetails(string number);
    void MarkProcessedPurchaseOrderDetail(int id);
}

So here's the App class that encapsulates the whole logic:

```
public class App
{
private static readonly string LastRevisionOptionalFieldName = PurchaseOrderHeader.OptionalFieldNames[PurchaseOrderHeader.OptionalField.RevisionDate];

private readonly IUnitOfWork _unitOfWork;
private readonly IPurchaseOrderLoader _loader;

public App(IUnitOfWork unitOfWork, IPurchaseOrderLoader loader)
{
_unitOfWork = unitOfWork;
_

Let's get straight to your main point about the bool ProcessHeader() method.

The first thing which popped in my head had been the multiple checks for null about existingHeader. Only the first if condition may do something if existingHeader == null so let us refactor this under the assumption we don't need the returned bool state, like so

if (existingHeader == null)
    {
        if (!header.IsDeleted)
        {
            CreateNewHeaderRecord(header);
        }
        return;
    }

leaving the remaining code like so

if (!(new HeaderModifiedComparer()).Equals(header, existingHeader) && !header.IsDeleted)
    {
        UpdateExistingHeader(existingHeader, header);
    }
    else if (header.IsDeleted)
    {
        repository.Remove(existingHeader);
    }

    if (header.Key == 0)
    {
        header.Key = existingHeader.Key;
    }

Now we switch the if conditions like so

if (header.IsDeleted)
    {
        repository.Remove(existingHeader);
    }
    else if (!(new HeaderModifiedComparer()).Equals(header, existingHeader))
    {
        UpdateExistingHeader(existingHeader, header);
    }

    if (header.Key == 0)
    {
        header.Key = existingHeader.Key;
    }

If we now take a look at the former comment // no need to process details, deleting the header will delete the details. this can boil down to

private void ProcessHeader(PurchaseOrderHeader header)
{
    var repository = _unitOfWork.Set();

    var existingHeader = repository.SingleOrDefault(e => e.Number == header.Number);

    if (existingHeader == null)
    {
        if (!header.IsDeleted)
        {
            CreateNewHeaderRecord(header);
        }
        return;
    }

    if (header.IsDeleted)
    {
        repository.Remove(existingHeader);
    }
    else if (!(new HeaderModifiedComparer()).Equals(header, existingHeader))
    {
        UpdateExistingHeader(existingHeader, header);
    }

    if (header.Key == 0)
    {
        header.Key = existingHeader.Key;
    } 
}

and now the calling code in question can look like so

ProcessHeader(header);
if (!header.IsDeleted)
{
    details = _loader.GetPurchaseOrderDetails(header.Number).OrderBy(e => e.LineNumber);
    ProcessDetails(details, header);
}

But IMO this doesn't look that good. Let us define an overloaded ProcessDetails(PurchaseOrderHeader) like so

private void ProcessDetails(PurchaseOrderHeader header)
{
    if (header.IsDeleted)
    {
        // no need to process details, deleting the header will delete the details.  
        return;
    }

    var details = _loader.GetPurchaseOrderDetails(header.Number).OrderBy(e => e.LineNumber).ToArray();
    ProcessDetails(details, header);
}

which leaves the Start() method like so

public void Start()
{
    // performing projection with Linq-to-Objects, since Linq-to-Sage won't handle this:
    var vendorCodes = new HashSet(_unitOfWork.Set().ToList().Select(e => e.Key));

    var headers = _loader.GetPurchaseOrderHeaders();
    foreach (var header in headers)
    {

        if (!vendorCodes.Contains(header.VendorCode))
        {
            Debug.WriteLine("PO #{0} is referring to invalid vendor code '{1}'; PO is being skipped.", header.Number, header.VendorCode);
            continue;
        }

        ProcessHeader(header);
        ProcessDetails(header);

        Debug.WriteLine("Committing transaction for PO #" + header.Number);
        try
        {
            _unitOfWork.SaveChanges();
            MarkProcessedEntities(header, details);
        }
        catch (SageSessionException exception)
        {
            Debug.WriteLine(exception);
        }
    }
}

The ProcessDetails() could use a facelifting as well. We should use the fact that if header.IsDeleted this method won't be executed at all and rearange the if conditions a little bit like so

private void ProcessDetails(IEnumerable details, PurchaseOrderHeader header)
{
    var existingDetails = GetExistingDetails(header.Number).ToList();

    foreach (var detail in details)
    {
        var existingDetail = existingDetails.SingleOrDefault(e => DetailModifiedComparer.LineNumberEquals(e.LineNumber, detail.LineNumber));
        if (existingDetail == null)
        {
            if (!detail.IsDeleted)
            {
                CreateNewDetailRecord(header, detail);
            }
            continue;
        }

        detail.PurchaseOrderHeaderKey = existingDetail.PurchaseOrderHeaderKey;

        if (detail.IsDeleted)
        {
            _unitOfWork.Set().Remove(detail);
        }
        else if (!(new DetailModifiedComparer()).Equals(detail, existingDetail))
        {
            UpdateExistingDetail(existingDetail, detail);
        }

    }
}

Instead of the new PurchaseOrderDetail[] {} here

```
private IEnumerable GetExistingDetails(string headerNumber)
{
var header = _unitOfWork.Set().SingleOrDefault(e => e.Number == headerNumber);
return head

StackExchange Code Review Q#131914, answer score: 3