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<T> Set<T>() 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<PurchaseOrderHeader> GetPurchaseOrderHeaders();
void MarkProcessedPurchaseOrderHeader(int id);
IEnumerable<PurchaseOrderDetail> 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;
_loader = loader;
}
public void Start()
{
// performing projection with Linq-to-Objects, since Linq-to-Sage won't handle this:
var vendorCodes = new HashSet<string>(_unitOfWork.Set<Vendor>().ToList().Select(e => e.Key));
var headers = _loader.GetPurchaseOrderHeaders();
foreach (var header in headers)
{
IEnumerable<PurchaseOrderDetail> details = new PurchaseOrderDetail[] { };
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;
}
if (ProcessHeader(header))
{
details = _loader.GetPurchaseOrderDetails(header.Number).OrderBy(e => e.LineNumber);
ProcessDetails(details, header);
}
try
{
Debug.WriteLine("Committing transaction for PO #" + header.Number);
_unitOfWork.SaveChanges();
MarkProcessedEntities(header, details);
}
catch (SageSessionException exception)
{
Debug.WriteLine(exception);
}
}
}
private bool ProcessHeader(PurchaseOrderHeader header)
{
var repository = _unitOfWork.Set<PurchaseOrderHeader>();
var result = true;
var existingHeader = repository.SingleOrDefault(e => e.Number == header.Number);
if (existingHeader == null && !header.IsDeleted)
{
CreateNewHeaderRecord(header);
}
else if (existingHeader != null && !(new HeaderModifiedComparer()).Equals(header, existingHeader) && !header.IsDeleted)
{
UpdateExistingHeader(existingHeader, header);
}
else if (existingHeader != null && header.IsDeleted)
{
repository.Remove(existingHeader);
result = false; // no need to process details, deleting the header will delete the details.
}
if (header.Key == 0 && existingHeader != null)
{
header.Key = existingHeader.Key;
}
return result;
}
private void ProcessDetails(IEnumerable<PurchaseOrderDetail> 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)
{
detail.PurchaseOrderHeaderKey = existingDetail.PurchaseOrderHeaderKey;
}
if (existingDetail == null && !detail.IsDeleted)
{
CreateNewDetailRecord(header, detail);
}
else if (!(new DetailModifiedComparer()).Equals(detail, existingDetail) && !detail.IsDeleted)
{
UpdateExistingDetail(existingDetail, detail);
}
else if (existingDetail != null && detail.IsDeleted && !header.IsDeleted)
{
_unitOfWork.Set<PurchaseOrderDetail>().Remove(detail);
}
}
}
private IEnumerable<PurchaseOrderDetail> GetExistingDetails(string headerNumber)
{
var header = _unitOfWork.Set<PurchaseOrderHeader>().SingleOrDefault(e => e.Number == headerNumber);
return header == null
? new PurchaseOrderDetail[] {}
: header.Details;
}
private void CreateNewHeaderRecord(PurchaseOrderHeader header)
{
// "last modified" date is stored in an optional field:
var revisionDate = new PO0623
{
FieldName = LastRevisionOptionalFieldName,
FieldType = OptionalFieldType.Date,
DateValue = DateTime.Now
};
header.OptionalFields.Add(revisionDate);
_unitOfWork.Set<PurchaseOrderHeader>().Add(header);
}
private void UpdateExistingHeader(PurchaseOrderHeader existingHeader, PurchaseOrderHeader header)
{
// PO header exists in Sage AND is deemed modified: update it.
existingHeader.FreeOnBoardPoint = header.FreeOnBoardPoint;
existingHeader.IsOnHold = header.IsOnHold;
existingHeader.OrderDate = header.OrderDate;
existingHeader.Type = header.Type;
var revisionDate = existingHeader.OptionalFields.SingleOrDefault(e => e.FieldName == LastRevisionOptionalFieldName);
if (revisionDate != null)
{
revisionDate.DateValue = DateTime.Now;
}
}
private void CreateNewDetailRecord(PurchaseOrderHeader header, PurchaseOrderDetail detail)
{
detail.PurchaseOrderHeaderKey = header.Key;
header.Details.Add(detail);
// Linq-to-Sage entity tracker doesn't subclass entity types,
// so it can't know about the added entity and thus we need to attach the detail entity manually:
_unitOfWork.Set<PurchaseOrderDetail>().Attach(detail, EntityState.Added);
}
private void UpdateExistingDetail(PurchaseOrderDetail existingDetail, PurchaseOrderDetail detail)
{
existingDetail.QuantityOrdered = detail.QuantityOrdered;
existingDetail.UnitWeight = detail.UnitWeight;
existingDetail.UnitCost = detail.UnitCost;
}
private void MarkProcessedEntities(PurchaseOrderHeader header, IEnumerable<PurchaseOrderDetail> details)
{
_loader.MarkProcessedPurchaseOrderHeader(header.Id);
foreach (var detail in details)
{
_loader.MarkProcessedPurchaseOrderDetail(detail.Id);
}
}
}
All this code was originally implemented as static methods in the Program class, so as you can see the refactoring is well under way.
Aside from some annoying things I need to fix in my LINQ provider, there's a specific thing I'm not very happy about:
if (ProcessHeader(header)) { details = _loader.GetPurchaseOrderDetails(header.Number).OrderBy(e => e.LineNumber); ProcessDetails(details, header); }
If, while processing the header, I notice I have nothing to do with detail records, I just skip them. The problem is with how that was implemented: I don't like that I have a bool ProcessHeader and then a void ProcessDetails method. Is there not a better way to do this?
Anything else looks weird? Say I'd want to write a thorough test suite for this code, should I extract more types? Is App doing too many things?
1 Answer 1
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<PurchaseOrderHeader>();
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<string>(_unitOfWork.Set<Vendor>().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<PurchaseOrderDetail> 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<PurchaseOrderDetail>().Remove(detail);
}
else if (!(new DetailModifiedComparer()).Equals(detail, existingDetail))
{
UpdateExistingDetail(existingDetail, detail);
}
}
}
Instead of the new PurchaseOrderDetail[] {} here
private IEnumerable<PurchaseOrderDetail> GetExistingDetails(string headerNumber) { var header = _unitOfWork.Set<PurchaseOrderHeader>().SingleOrDefault(e => e.Number == headerNumber); return header == null ? new PurchaseOrderDetail[] {} : header.Details; }
you should consider to use Enumerable.Empty<PurchaseOrderDetail>.