0
\$\begingroup\$

I have a business logic layer class for creating and updating work orders, it also retrieves existing work order details from the database and displays it to the user on a form. The business logic layer class uses DTOs defined in a separate Class Library

Inspiration for using DTOs was found here, although I am sticking to using DataSets and DataTables (which linked article strongly disagrees with). I have taken replies to my previous code review question in consideration while coding up this form in hopes of reusing the pattern in other parts of my project.

Here are the samples from the front end for creating, updating and retrieving.

        //INSERT/UPDATE WORK ORDER
    public void btnFinish_Click(object sender, EventArgs e)
    {
        try
        {
            //hardcoded values for PLANT and SITE to be fetched from user login controller properties.
            //LoginController.UserLocation + LoginController.UserSite
            AuxController woc = new AuxController();
            if (FormChecker.IsValidForm(Controls.Cast<Control>().ToList()))
            {
                    //NB NB NB PERCENTAGE SMYS DURATION:
                    //STRING BUILDING: "BAR VALUE FROM txtBAR" + " @ " + "COMBOBOX SELECTED SECONDS" + " seconds"
                    woc.CreateNewWorkOrder(txtSysproOrderNum.Text, txtContractNo.Text, PT_BLL.Controllers.Login.LoginController.UserPlant, txtSysproStockCode.Text,
                    txtCustomerName.Text, Convert.ToInt32(numQty.Value), numMeters.Value, cmbType.SelectedItem.ToString(), cmbSpec.SelectedItem.ToString(),
                    cmbSteelGrade.SelectedItem.ToString(), dtpOrderDate.Value, dtpDueDate.Value, numLength.Value, PT_BLL.Controllers.Login.LoginController.UserSite,
                    numDiameter.Value, numThickness.Value, "N/A", "N/A", "N/A", "N/A", txtBar.Text + " @ " + cmbSeconds.SelectedItem.ToString() + " seconds",
                    PT_BLL.Controllers.Login.LoginController.UserNameLoggedIn, chkHydroTest.Checked, numLengthPlus.Value, numLengthMinus.Value,
                    numShortLengthPerc.Value, numBevMin.Value, numBevMax.Value, numBodDiaMin.Value, numBodDiaMax.Value, numPEDiaMin.Value,
                    numPEDiaMax.Value, chkExport.Checked.ToString(), btnFinish.BackColor != System.Drawing.Color.ForestGreen);
            }
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
        }
    }

    //RETRIEVE A JOB NUMBER/ENTER NOW JOB NUMBER AFTER USER LEAVES TEXTBOX
    private void txtSysproOrderNum_Leave(object sender, EventArgs e)
    {
        try
        {
            AuxController woc = new AuxController();
            var existingWorkOrder = woc.GetJobDetails(txtSysproOrderNum.Text);
            if (!existingWorkOrder.IsNew)
            {
                //IF EXISTING DATA, DISPLAY LABEL WITH DATE AND USER WHO CAPTURED EXISTING WOP DATA.
                //ALSO CHANGE BUTTON COLOUR TO ORANGERED FOR UPDATE CASE
                //change button colour
                btnFinish.BackColor = System.Drawing.Color.OrangeRed;
                lblWOPStatus.Text = "WOP STATUS: CAPTURED ON " + existingWorkOrder.OrderDateCreated + " BY " + existingWorkOrder.Operator;
                txtSysproStockCode.Text = existingWorkOrder.SysproStockCode;
                txtCustomerName.Text = existingWorkOrder.Customer;
                chkExport.Checked = existingWorkOrder.OrderType.Contains("EXPORT");
                numQty.Value = existingWorkOrder.Quantity;
                dtpOrderDate.Value = existingWorkOrder.OrderDate;
                dtpDueDate.Value = existingWorkOrder.DueDate;
                txtContractNo.Text = existingWorkOrder.ContractNumber;
                cmbType.SelectedItem = existingWorkOrder.PipeType;
                cmbSpec.SelectedItem = existingWorkOrder.SpecificationERW;
                cmbSteelGrade.SelectedItem = existingWorkOrder.SteelGrade;
                chkHydroTest.Checked = Convert.ToBoolean(existingWorkOrder.HydroTest);
                numDiameter.Value = existingWorkOrder.OuterDiameterERW;
                numThickness.Value = existingWorkOrder.WallThicknessERW;
                numLength.Value = existingWorkOrder.Length;
                numMeters.Value = existingWorkOrder.Length * existingWorkOrder.Quantity;
                numLengthMinus.Value = existingWorkOrder.VarianceLengthMinus;
                numLengthPlus.Value = existingWorkOrder.VarianceLengthPlus;
                numShortLengthPerc.Value = existingWorkOrder.ShortLengthVariance;
                numBevMin.Value = existingWorkOrder.BevelAnglesMin;
                numBevMax.Value = existingWorkOrder.BevelAnglesMax;
                numBodDiaMin.Value = existingWorkOrder.BodyDiameterMin;
                numBodDiaMax.Value = existingWorkOrder.BodyDiameterMax;
                numPEDiaMin.Value = existingWorkOrder.PipeEndDiameterMin;
                numPEDiaMax.Value = existingWorkOrder.PipeEndDiameterMax;
                txtBar.Text = existingWorkOrder.Bar.ToString();
                cmbSeconds.SelectedItem = existingWorkOrder.HoldingTime.ToString();
            }
            else
            {
                string preserveText = txtSysproOrderNum.Text;
                //GENERAL CLASS FOR CLEARING WINFORMS.
                FormChecker.ClearAll(this);
                txtSysproOrderNum.Text = preserveText;
                btnFinish.BackColor = System.Drawing.Color.ForestGreen;
                //LABEL CHANGES WHEN USER ACCESS EXISTING RECORD AND FORM POPULATES WITH EXISTING DATA.
                //SET LABEL BACK TO ORIGINAL VALUE
                lblWOPStatus.Text = "WOP Status: NOT CAPTURED ";
            }

        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
        }
    }

Here is my business layer class structure for above actions taken in the front end/ UI:

        public void CreateNewWorkOrder(string SysproOrderNumber, string ContractNumber, string Plant, string SysproStockCode,
                string Customer, int Quantity, decimal Meters, string PipeType, string SpecificationERW, string SteelGrade, DateTime OrderDate,
                DateTime DueDate, decimal Length, string Site, decimal OuterDiameterERW, decimal WallThicknessERW, string Coating, string Lining, string CutBack,
                string CutBackType, string PercentageSMYSDuration, string Operator, bool HydroTest, decimal VarianceLengthPlus, decimal VarianceLengthMinus,
                decimal ShortLengthVariance, decimal BevelAnglesMin, decimal BevelAnglesMax, decimal BodyDiameterMin, decimal BodyDiameterMax,
                decimal PipeEndDiameterMin, decimal PipeEndDiameterMax, string OrderType, bool UpdateWOP)
    {
        dto_WorkOrder wop = new dto_WorkOrder();
        try
        {
            wop.SysproOrderNumber = SysproOrderNumber;
            wop.ContractNumber = ContractNumber;
            wop.Plant = Plant;
            wop.SysproStockCode = SysproStockCode;
            wop.Customer = Customer;
            wop.Quantity = Quantity;
            wop.Meters = Meters;
            wop.PipeType = PipeType;
            wop.SpecificationERW = SpecificationERW;
            wop.SteelGrade = SteelGrade;
            wop.OrderDate = OrderDate;
            wop.DueDate = DueDate;
            wop.Length = Length;
            wop.Site = Site;
            wop.OuterDiameterERW = OuterDiameterERW;
            wop.WallThicknessERW = WallThicknessERW;
            wop.Coating = Coating;
            wop.Lining = Lining;
            wop.CutBack = CutBack;
            wop.CutBackType = CutBackType;
            wop.PercentageSMYSDuration = PercentageSMYSDuration;
            wop.Operator = Operator;
            wop.HydroTest = HydroTest;
            wop.VarianceLengthPlus = VarianceLengthPlus;
            wop.VarianceLengthMinus = VarianceLengthMinus;
            wop.ShortLengthVariance = ShortLengthVariance;
            wop.BevelAnglesMin = BevelAnglesMin;
            wop.BevelAnglesMax = BevelAnglesMax;
            wop.BodyDiameterMin = BodyDiameterMin;
            wop.BodyDiameterMax = BodyDiameterMax;
            wop.PipeEndDiameterMin = PipeEndDiameterMin;
            wop.PipeEndDiameterMax = PipeEndDiameterMax;
            wop.OrderType = Convert.ToBoolean(OrderType) ? "EXPORT" : "LOCAL";
            if (UpdateWOP)
            {
                bool successUpdate = UpdateWorkOrder(wop);
                MessageBox.Show(successUpdate ? "SUCCESS: WORKORDER " + wop.SysproOrderNumber + " HAS BEEN UPDATED." : "FAIL: RECORD WAS NOT UPDATED");

            }
            else
            {
                bool successInsert = InsertWorkOrder(wop);
                MessageBox.Show(successInsert ? "SUCCESS: WORKORDER " + wop.SysproOrderNumber + " HAS BEEN INSERTED." : "FAIL: RECORD WAS NOT INSERTED");
            }
            //TO DO: Should implement dialogresult for capturing new record, to conditionally clear screen or review data inserted.
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
        }
    }


        private WorkOrderTableAdapter _wop = null;

    protected WorkOrderTableAdapter AdapterWop
    {
        get
        {
            if (_wop == null)

                _wop = new WorkOrderTableAdapter();
            _wop.ConnectionString = Login.LoginController.UserConnString;
            return _wop;
        }
    }

    public dto_WorkOrder GetJobDetails(string jobnumber)
    {
        tblWorkOrderPlanningDataSet.WorkOrderDataTable jobDt;
        dto_WorkOrder workorder = new dto_WorkOrder();
        jobDt = AdapterWop.GetData(jobnumber);
        if (jobDt.Rows.Count > 0)
        {
            //set bool to control/ be aware of existing data in front end.
            workorder.IsNew = false;
            workorder.SysproOrderNumber = jobDt[0].Syspro_Order_Number;
            workorder.ContractNumber = jobDt[0].Contract_Number;
            workorder.Plant = jobDt[0].Plant;
            workorder.SysproStockCode = jobDt[0].Syspro_Stock_Code;
            workorder.CustomerPurchaseOrderNumber = jobDt[0].Customer_Purchase_Order_Number;
            workorder.CoilNumbers = jobDt[0].Coil_Number.Split(',').ToList<string>();
            workorder.Customer = jobDt[0].Customer;
            workorder.Quantity = jobDt[0].Quantity;
            workorder.Meters = Convert.ToDecimal(jobDt[0].Meters);
            workorder.PipeType = jobDt[0].Pipe_Type;
            workorder.SpecificationERW = jobDt[0].Specification_ERW;
            workorder.SteelGrade = jobDt[0].Steel_Grade;
            workorder.OrderDate = Convert.ToDateTime(jobDt[0].Order_Date);
            workorder.DueDate = Convert.ToDateTime(jobDt[0].Due_Date);
            workorder.Length = Convert.ToDecimal(jobDt[0].Length);
            workorder.Site = jobDt[0].Site;
            workorder.OuterDiameterERW = Convert.ToDecimal(jobDt[0].Outer_Diameter_ERW.ToString());
            workorder.WallThicknessERW = Convert.ToDecimal(jobDt[0].Wall_Thickness_ERW.ToString());
            workorder.Coating = jobDt[0].Coating;
            workorder.Lining = jobDt[0].Lining;
            workorder.CutBack = jobDt[0].Cut_Back;
            workorder.CutBackType = jobDt[0].Cut_Back_Type;
            //BREAK UP STRING PERCENTAGESMYSDURATION TO GET HOLDING TIME + BAR/PRESSURE INTO SEPERATE BOXES/DROPDOWNS/CHECKBOXES
            workorder.PercentageSMYSDuration = jobDt[0].Percentage_SMYS_Duration;
            try
            {
                int ixBar = workorder.PercentageSMYSDuration.IndexOf('@');
                int ixSecond = workorder.PercentageSMYSDuration.IndexOf('s');
                workorder.Bar = Convert.ToDecimal(workorder.PercentageSMYSDuration.Substring(0, ixBar - 1));
                workorder.HoldingTime = Convert.ToDecimal(workorder.PercentageSMYSDuration.Trim().Substring(ixBar + 1, ixSecond - ixBar - 1).Trim());
            }
            catch (Exception ex)
            {
                MessageBox.Show("HYDRO PERCENTAGE WAS CAPTURED INCORRECTLY." + " \r\n" + "PLEASE SEE BOX NEXT TO %SMYS DURATION FOR FULL HYDRO PRESSURE/HOLDING TIME" + "\r\n" + "THIS IS EXPECTED TO HAPPEN FOR OLDER JOBS." + "\r\n" + ex.Message);
            }
            workorder.Operator = jobDt[0].Operator;
            workorder.Remaining = Convert.ToDecimal(jobDt[0].Remaining);
            workorder.HydroTest = Convert.ToBoolean(jobDt[0].HydroTest);
            workorder.VarianceLengthPlus = Convert.ToDecimal(jobDt[0].Variance_Length_Plus);
            workorder.VarianceLengthMinus = Convert.ToDecimal(jobDt[0].Variance_Length_Minus);
            workorder.ShortLengthVariance = Convert.ToDecimal(jobDt[0].ShortLength_Variance);
            workorder.BevelAnglesMin = Convert.ToDecimal(jobDt[0].Bevel_Angles_Min);
            workorder.BevelAnglesMax = Convert.ToDecimal(jobDt[0].Bevel_Angles_Max);
            workorder.BodyDiameterMin = Convert.ToDecimal(jobDt[0].Body_Diameter_Min);
            workorder.BodyDiameterMax = Convert.ToDecimal(jobDt[0].Body_Diameter_Max);
            workorder.PipeEndDiameterMin = Convert.ToDecimal(jobDt[0].Pipe_End_Diameter_Min);
            workorder.PipeEndDiameterMax = Convert.ToDecimal(jobDt[0].Pipe_End_Diameter_Max);
            workorder.OrderType = jobDt[0].Order_Type;
            if (String.IsNullOrEmpty(workorder.OrderType))
            {
                workorder.OrderType = "UNKNOWN";
            }
            else
            {
                if (workorder.OrderType.Contains("LOCAL"))
                {
                    workorder.OrderType = jobDt[0].Order_Type;
                }
                else
                {
                    if (workorder.OrderType.Contains("EXPORT"))
                    {
                        workorder.OrderType = jobDt[0].Order_Type;
                    }
                    //probably an unneccessary else, user selects checkboxes so shouldn't end up here at all.
                    //cater for old data which may have bad characteristics l0cal exp0rt or incorrect spelling.
                    else
                    {
                        workorder.OrderType = "UNKNOWN";
                    }
                }
            }
        }
        else
        {
            workorder.IsNew = true;
        }
        return workorder;
    }

    public bool InsertWorkOrder(dto_WorkOrder wop)
    {
        bool successInsert = true;
        try
        {
            AdapterWop.Insert(wop.SysproOrderNumber, wop.ContractNumber, wop.Plant, wop.SysproStockCode,
                wop.Customer, wop.Quantity, wop.Meters, wop.PipeType, wop.SpecificationERW, wop.SteelGrade, wop.OrderDate,
                wop.DueDate, wop.Length, wop.Site, wop.OuterDiameterERW, wop.WallThicknessERW, wop.Coating, wop.Lining, wop.CutBack,
                wop.CutBackType, wop.PercentageSMYSDuration, wop.Operator, wop.HydroTest, wop.VarianceLengthPlus, wop.VarianceLengthMinus,
                wop.ShortLengthVariance, wop.BevelAnglesMin, wop.BevelAnglesMax, wop.BodyDiameterMin, wop.BodyDiameterMax,
                wop.PipeEndDiameterMin, wop.PipeEndDiameterMax, wop.OrderType);
            successInsert = true;
            return successInsert;
        }
        catch (SqlException ex)
        {
            MessageBox.Show(ex.Message + "\r\n PLEASE CONTACT BRENDAN.");
            successInsert = false;
            return successInsert;
        }
    }

    private bool UpdateWorkOrder(dto_WorkOrder wop)
    {
        bool successUpdate = true;
        try
        {
            AdapterWop.Update(wop.SysproStockCode, wop.Customer, wop.Quantity, wop.Meters, wop.PipeType,
                            wop.SpecificationERW, wop.SteelGrade, wop.OrderDate, wop.DueDate, wop.Length, wop.OuterDiameterERW,
                            wop.WallThicknessERW, wop.PercentageSMYSDuration, wop.Operator, wop.HydroTest, wop.VarianceLengthPlus,
                            wop.VarianceLengthMinus, wop.ShortLengthVariance, wop.BevelAnglesMin, wop.BevelAnglesMax,
                            wop.BodyDiameterMin, wop.BodyDiameterMax, wop.PipeEndDiameterMin, wop.PipeEndDiameterMax,
                            wop.OrderType, wop.SysproOrderNumber, wop.ContractNumber);
            return successUpdate;

        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message + "\r\n PLEASE CONTACT BRENDAN.");
            successUpdate = false;
            return successUpdate;
        }
    }

My WorkOrder_DTO class:

    public class dto_WorkOrder : dto_Base
{
    public string SysproOrderNumber { get; set; }
    public string ContractNumber { get; set; }
    public string Plant { get; set; }
    public string SysproStockCode { get; set; }
    public string CustomerPurchaseOrderNumber { get; set; }
    public List<string> CoilNumbers { get; set; }
    public string Customer { get; set; }
    public int Quantity { get; set; }
    public decimal Meters { get; set; }
    public string PipeType { get; set; }
    public string SpecificationERW { get; set; }
    public string SteelGrade { get; set; }
    public DateTime OrderDate { get; set; }
    public DateTime DueDate { get; set; }
    public decimal Length { get; set; }
    public string Site { get; set; }
    public decimal OuterDiameterERW { get; set; }
    public decimal WallThicknessERW { get; set; }
    public string Coating { get; set; }
    public string Lining { get; set; }
    public string CutBack { get; set; }
    public string CutBackType { get; set; }
    public string PercentageSMYSDuration { get; set; }
    public decimal Bar { get; set; } 
    public decimal HoldingTime { get; set; }
    public string Operator { get; set; }
    public decimal Remaining { get; set; }
    public bool? HydroTest { get; set; }
    public decimal VarianceLengthPlus { get; set; }
    public decimal VarianceLengthMinus { get; set; }
    public decimal ShortLengthVariance { get; set; }
    public decimal BevelAnglesMin { get; set; }
    public decimal BevelAnglesMax { get; set; }
    public decimal BodyDiameterMin { get; set; }
    public decimal BodyDiameterMax { get; set; }
    public decimal PipeEndDiameterMin { get; set; }
    public decimal PipeEndDiameterMax { get; set; }
    public string OrderType { get; set; }
    public DateTime OrderDateCreated { get; set; }

    public dto_WorkOrder()
    {
        SysproOrderNumber = String_NullValue;
        ContractNumber = String_NullValue;
        Plant = String_NullValue;
        SysproStockCode = String_NullValue;
        CustomerPurchaseOrderNumber = String_NullValue;
        CoilNumbers = List_NullValue;
        Customer = String_NullValue;
        Quantity = Int_NullValue;
        Meters = Decimal_NullValue;
        PipeType = String_NullValue;
        SpecificationERW = String_NullValue;
        SteelGrade = String_NullValue;
        OrderDate = DateTime_NullValue;
        DueDate = DateTime_NullValue;
        Length = Decimal_NullValue;
        Site = String_NullValue;
        OuterDiameterERW = Decimal_NullValue;
        WallThicknessERW = Decimal_NullValue;
        Coating = String_NullValue;
        Lining = String_NullValue;
        CutBack = String_NullValue;
        CutBackType = String_NullValue;
        PercentageSMYSDuration = String_NullValue;
        Operator = String_NullValue;
        Remaining = Decimal_NullValue;
        HydroTest = Bool_NullValue;
        VarianceLengthPlus = Decimal_NullValue;
        VarianceLengthMinus = Decimal_NullValue;
        ShortLengthVariance = Decimal_NullValue;
        BevelAnglesMin = Decimal_NullValue;
        BevelAnglesMax = Decimal_NullValue;
        BodyDiameterMin = Decimal_NullValue;
        BodyDiameterMax = Decimal_NullValue;
        PipeEndDiameterMin = Decimal_NullValue;
        PipeEndDiameterMax = Decimal_NullValue;
        OrderType = String_NullValue;
        Bar = Decimal_NullValue;
        HoldingTime = Decimal_NullValue;
        OrderDateCreated = DateTime_NullValue;
    }
}

I appreciate the feedback and as far as I can tell, this seems like a good way of going about wiring up the rest of the application to ensure single responsibility and achieve 3 tier architecture.

TLDR: The DAL (DataSets) runs queries and returns DataTables to the BLL (Business Logic Layer). The BLL accepts basic arguments from the front end in its methods to get/insert/update data using constructed DTOs from the resulting DataTables. Good Practice?

\$\endgroup\$
  • \$\begingroup\$ Edits: Forgot to add method CreateNewWorkOrder in BLL code block. \$\endgroup\$ – Brendan Jacobs Mar 8 at 9:30
  • 1
    \$\begingroup\$ What's with all the _NullValues? What is this? \$\endgroup\$ – t3chb0t Mar 8 at 13:51
4
\$\begingroup\$

Some quick remarks:

  • Quite frankly, I see no point for CreateNewWorkOrder and its 30-something parameters (a maintenance nightmare). Simply create the dto_WorkOrder where you call CreateNewWorkOrder and then pass that dto on to a method that performs an insert or an update.

  • You also can't claim that something is part of the business layer when that method has calls to MessageBox.Show inside it. Let that method (for instance) return a custom class featuring a boolean (to indicate success or failure) and an optional error message.

  • Move away from DataTables etc. Instead use an ORM like Dapper.

  • Class names and property names etc. should not contain underscores. Please follow Microsoft's guidelines.

  • What are String_NullValue and List_NullValue and Int_NullValue etc.? Please do not pollute your constructors with needlessly complex default values.

  • Much of this code feels "ancient". You're not even using String.Format, and that is already usually replaced by $ - string interpolation.

  • Take a look at the Model View Presenter Pattern to write cleaner WinForms code.

\$\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.