patterncsharpMinor

Purchase-ordering

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

orderingpurchasestackoverflow

Problem

This is the Save function on my PurchaseOrder model in my real world production system.

This code needs to write the state of the model to the database schema. It's grown over time as things have been added, so I'd like to work it a bit cleaner.

As you can see, it's gotten very messy and is hard to work with now. Do you think I would be best to split Save into two functions a CreateNew and Update?

```
internal void Save()
{
int TotalJobs = 0;

foreach (int c in Contracts)
{
Contract contract = Contract.LoadId(c);
TotalJobs += contract.Quantity;
}

using (SqlConnection conn = new SqlConnection(ConnectionStrings.ConnectionString))
{
if (PurchaseOrderId == null)
{
DynamicParameters p = new DynamicParameters(new
{
CompanyId,
RecipientPersonId,
CurrencyId,
AddedBy = HttpContext.Current.User.Identity.Name,
InitialOrderDate,
ApprovedByPersonId,
DeliveryDetail,
EmailCC,
DeliveryTypeId,
DeliveryDateSingle,
ScheduleDaysBeforePlan,
WarrantyId,
RepairId
});
p.Add("PurchaseOrderId", 0, direction: ParameterDirection.Output);
p.Add("CreateByPersonId", 0, direction: ParameterDirection.Output);
conn.Execute("material.PurchaseOrder_Add", p, commandType: CommandType.StoredProcedure);
PurchaseOrderId = p.Get("PurchaseOrderId");
CreateByPersonId = p.Get("CreateByPersonId");
foreach (PurchaseOrderLine l in Lines)
{
l.PurchaseOrderId = (int)PurchaseOrderId;
l.Save(TotalJobs, Contracts, CompanyId); // This adds the new line.
}

if (Contracts.Count > 0)
{
foreach (int c in Contracts)
{

Solution

Do you think I would be best to split Save into two functions a CreateNew and Update?

A very quick and loud : YES !

This method is just to long. By splitting it into GetTotalJobs(), Update(int, int) and Save(int) you will make your code easier to read and maintain.

The former Save() method would then look like so

internal void Save()
{
    int totalJobs = GetTotalJobs();

    if (PurchaseOrderId == null)  
    {
        Save(totalJobs);
    }
    else
    {
        Update(PurchaseOrderId, totalJobs);
    }

}

with the GetTotalJobs() method looking like this

private int GetTotalJobs()
{
    int totalJobs = 0;

    foreach (int c in Contracts)
    {
        Contract contract = Contract.LoadId(c);
        totalJobs += contract.Quantity;
    }

    return totalJobs;
}

Based on the NET naming guidelines I changed the casing of the TotalJobs variable name to use camelCase casing.

But this little piece of code shows another strange thing. We are iterating over what seems a collection of int which is named Contracts and we use the values to call Contract.LoadId(). So changing the name of the property/field from Contracts to ContractIds and LoadId() to LoadById() will make things clearer for anyone who is reading/maintaining this code.

This will leave (just copied out of the former Save() method) the Save(int) method to

private void Save(int totalJobs)
{

    using (SqlConnection conn = new SqlConnection(ConnectionStrings.ConnectionString))
    {
        DynamicParameters p = new DynamicParameters(new
        {
            CompanyId,
            RecipientPersonId,
            CurrencyId,
            AddedBy = HttpContext.Current.User.Identity.Name,
            InitialOrderDate,
            ApprovedByPersonId,
            DeliveryDetail,
            EmailCC,
            DeliveryTypeId,
            DeliveryDateSingle,
            ScheduleDaysBeforePlan,
            WarrantyId,
            RepairId
        });
        p.Add("PurchaseOrderId", 0, direction: ParameterDirection.Output);
        p.Add("CreateByPersonId", 0, direction: ParameterDirection.Output);
        conn.Execute("material.PurchaseOrder_Add", p, commandType: CommandType.StoredProcedure);
        PurchaseOrderId = p.Get("PurchaseOrderId");
        CreateByPersonId = p.Get("CreateByPersonId");
        foreach (PurchaseOrderLine l in Lines)
        {
            l.PurchaseOrderId = (int)PurchaseOrderId;
            l.Save(totalJobs, Contracts, CompanyId);       // This adds the new line.
        }

        if (Contracts.Count > 0)
        {
            foreach (int c in Contracts)
            {
                if (c == 999999)
                    continue;

                conn.Execute("material.PurchaseOrderContract_Update", new { PurchaseOrderId, ContractId = c }, commandType: CommandType.StoredProcedure);
            }

            if (!AllocateExcessToStock)
            {
                conn.Execute("material.PurchaseOrder_UpdateBOM", new { PurchaseOrderId }, commandType: CommandType.StoredProcedure);
            }
            else
            {
                conn.Execute("DELETE FROM prod.BillOfMaterialLine WHERE PurchaseOrderId = @PurchaseOrderId", new { PurchaseOrderId });
            }
        }

        if (WarrantyId > 0)
        {
            foreach (PurchaseOrderLine l in _lines)
            {
                conn.Execute("INSERT INTO [prod].[WarrantyPart] ([WarrantyId], Description, Quantity, UnitPrice, PurchaseOrderLineId, AddedByPersonId) VALUES (@WarrantyId, @Description, @Quantity, @UnitPrice, @PurchaseOrderLineId, @AddedByPersonId)", new { WarrantyId, Description = l.Description, Quantity = l.Quantity, UnitPrice = l.UnitPrice, PurchaseORderLineId = l.PurchaseOrderDetailId, AddedByPersonId = CurrentUser.Get().PersonId });
            }
        }

        if (RepairId > 0)
        {
            foreach (PurchaseOrderLine l in _lines)
            {
                conn.Execute("INSERT INTO [prod].[RepairPart] ([RepairId], Description, Quantity, UnitPrice, PurchaseOrderLineId) VALUES (@RepairId, @Description, @Quantity, @UnitPrice, @PurchaseOrderLineId)", new { RepairId, Description = l.Description, Quantity = l.Quantity, UnitPrice = l.UnitPrice, PurchaseORderLineId = l.PurchaseOrderDetailId });
            }
        }

        foreach (var line in Lines)
        {
            if (line.DeliverySchedule != null && !line.OrigionalDetailId.HasValue)
            {
                foreach (var plannedDelivery in line.DeliverySchedule)
                {
                    plannedDelivery.PurchaseOrderDetailId = line.PurchaseOrderDetailId;
                    plannedDelivery.Add();
                }
            }
        }
    }
}

Here I will only comment on the obvious and won't provide a rewrite because there isn't enough domain specific context avaible.

-
add some vertical spacing to structure your code some more, hence increasing readability. For instance t

Code Snippets

internal void Save()
{
    int totalJobs = GetTotalJobs();

    if (PurchaseOrderId == null)  
    {
        Save(totalJobs);
    }
    else
    {
        Update(PurchaseOrderId, totalJobs);
    }

}

private int GetTotalJobs()
{
    int totalJobs = 0;

    foreach (int c in Contracts)
    {
        Contract contract = Contract.LoadId(c);
        totalJobs += contract.Quantity;
    }

    return totalJobs;
}

private void Save(int totalJobs)
{

    using (SqlConnection conn = new SqlConnection(ConnectionStrings.ConnectionString))
    {
        DynamicParameters p = new DynamicParameters(new
        {
            CompanyId,
            RecipientPersonId,
            CurrencyId,
            AddedBy = HttpContext.Current.User.Identity.Name,
            InitialOrderDate,
            ApprovedByPersonId,
            DeliveryDetail,
            EmailCC,
            DeliveryTypeId,
            DeliveryDateSingle,
            ScheduleDaysBeforePlan,
            WarrantyId,
            RepairId
        });
        p.Add("PurchaseOrderId", 0, direction: ParameterDirection.Output);
        p.Add("CreateByPersonId", 0, direction: ParameterDirection.Output);
        conn.Execute("material.PurchaseOrder_Add", p, commandType: CommandType.StoredProcedure);
        PurchaseOrderId = p.Get<int>("PurchaseOrderId");
        CreateByPersonId = p.Get<int>("CreateByPersonId");
        foreach (PurchaseOrderLine l in Lines)
        {
            l.PurchaseOrderId = (int)PurchaseOrderId;
            l.Save(totalJobs, Contracts, CompanyId);       // This adds the new line.
        }



        if (Contracts.Count > 0)
        {
            foreach (int c in Contracts)
            {
                if (c == 999999)
                    continue;

                conn.Execute("material.PurchaseOrderContract_Update", new { PurchaseOrderId, ContractId = c }, commandType: CommandType.StoredProcedure);
            }

            if (!AllocateExcessToStock)
            {
                conn.Execute("material.PurchaseOrder_UpdateBOM", new { PurchaseOrderId }, commandType: CommandType.StoredProcedure);
            }
            else
            {
                conn.Execute("DELETE FROM prod.BillOfMaterialLine WHERE PurchaseOrderId = @PurchaseOrderId", new { PurchaseOrderId });
            }
        }

        if (WarrantyId > 0)
        {
            foreach (PurchaseOrderLine l in _lines)
            {
                conn.Execute("INSERT INTO [prod].[WarrantyPart] ([WarrantyId], Description, Quantity, UnitPrice, PurchaseOrderLineId, AddedByPersonId) VALUES (@WarrantyId, @Description, @Quantity, @UnitPrice, @PurchaseOrderLineId, @AddedByPersonId)", new { WarrantyId, Description = l.Description, Quantity = l.Quantity, UnitPrice = l.UnitPrice, PurchaseORderLineId = l.PurchaseOrderDetailId, AddedByPersonId = CurrentUser.Get().PersonId });
            }
        }

        if (RepairId > 0)
        {
            foreach (PurchaseOrderLine l in _lines)
            {
                conn.Execute("INSERT INTO [prod].[RepairPart] ([RepairId], Description, Quantity, UnitPrice, PurchaseOrderLineId) VALUES (@RepairId, @Description, @Quantity, @UnitPrice, @PurchaseOrderLineId)", new { RepairId, Description = l.Description, Quantity = l.Quantity, UnitPrice = l.UnitPrice, PurchaseORderLineId = l.PurchaseOrderDetailId });
            }
        }

        foreach (var line in Lines)
        {

DynamicParameters p = new DynamicParameters(new
 {
     CompanyId,
     RecipientPersonId,
     CurrencyId,
     AddedBy = HttpContext.Current.User.Identity.Name,
     InitialOrderDate,
     ApprovedByPersonId,
     DeliveryDetail,
     EmailCC,
     DeliveryTypeId,
     DeliveryDateSingle,
     ScheduleDaysBeforePlan,
     WarrantyId,
     RepairId
 });
 p.Add("PurchaseOrderId", 0, direction: ParameterDirection.Output);
 p.Add("CreateByPersonId", 0, direction: ParameterDirection.Output);
 conn.Execute("material.PurchaseOrder_Add", p, commandType: CommandType.StoredProcedure);
 PurchaseOrderId = p.Get<int>("PurchaseOrderId");
 CreateByPersonId = p.Get<int>("CreateByPersonId");
 foreach (PurchaseOrderLine l in Lines)
 {
      l.PurchaseOrderId = (int)PurchaseOrderId;
      l.Save(totalJobs, Contracts, CompanyId);       // This adds the new line.
 }

DynamicParameters p = new DynamicParameters(new
{
    CompanyId,
    RecipientPersonId,
    CurrencyId,
    AddedBy = HttpContext.Current.User.Identity.Name,
    InitialOrderDate,
    ApprovedByPersonId,
    DeliveryDetail,
    EmailCC,
    DeliveryTypeId,
    DeliveryDateSingle,
    ScheduleDaysBeforePlan,
    WarrantyId,
    RepairId
});

p.Add("PurchaseOrderId", 0, direction: ParameterDirection.Output);
p.Add("CreateByPersonId", 0, direction: ParameterDirection.Output);

conn.Execute("material.PurchaseOrder_Add", p, commandType: CommandType.StoredProcedure);

PurchaseOrderId = p.Get<int>("PurchaseOrderId");
CreateByPersonId = p.Get<int>("CreateByPersonId");

foreach (PurchaseOrderLine l in Lines)
{
    l.PurchaseOrderId = (int)PurchaseOrderId;
    l.Save(totalJobs, Contracts, CompanyId);       // This adds the new line.
}

Context

StackExchange Code Review Q#106981, answer score: 3

Revisions (0)

No revisions yet.