3
\$\begingroup\$

I am developing a report at the moment and I want to apply OOP principle because I feel like my code is not well written and there is duplication.

I have a method that gets data from the database and binds it to the controls on my report.

Here is a sample method I used to print preview a report:

public static void CreateReportResult(string poNumber)
{
    DataSet ds = new DataSet("PurchaseOrders");
    Dal.GetDataSet(ds, sqlQuery(poNumber));
    PurchaseReport purchaseOrderReport = new PurchaseReport
    {
        DataSource = ds,
        DataMember = "PurchaseOrders"
    };


    purchaseOrderReport.xrDateCreated.DataBindings.Add("Text", null, "PurchaseOrders.DateCreated", "{0:dddd, d MMMM, yyyy}");
    purchaseOrderReport.xrPO.DataBindings.Add("Text", null, "PurchaseOrders.PONumber", "{0}");
    purchaseOrderReport.xrVendor.DataBindings.Add("Text", null, "PurchaseOrders.POVendor", "{0}");
    purchaseOrderReport.xrPromocode.DataBindings.Add("Text", null, "PurchaseOrders.PromoCode", "{0}");
    //Bill Group
    //purchaseOrderReport.xrBusinessName.DataBindings.Add("Text", null, "PurchaseOrders.b_BillToGroup", "{0}");
    purchaseOrderReport.xrAddress1.DataBindings.Add("Text", null, "PurchaseOrders.b_AddressLine1", "{0}");
    purchaseOrderReport.xrAddress2.DataBindings.Add("Text", null, "PurchaseOrders.b_AddressLine2", "{0}");
    purchaseOrderReport.xrCityStateZip.DataBindings.Add("Text", null, "PurchaseOrders.CityStateZip", "{0}");
    purchaseOrderReport.xrPhone.DataBindings.Add("Text", null, "PurchaseOrders.b_Phone", "{0}");
    //Customer
    purchaseOrderReport.xrCustomerBusinessName.DataBindings.Add("Text", null, "PurchaseOrders.CustomerBusinessName", "{0}");
    purchaseOrderReport.xrCustomerName.DataBindings.Add("Text", null, "PurchaseOrders.CustomerName", "{0}");
    purchaseOrderReport.xrCustomerAddress.DataBindings.Add("Text", null, "PurchaseOrders.customerAddress", "{0}");
    purchaseOrderReport.xrCustomerCityStateZip.DataBindings.Add("Text", null, "PurchaseOrders.CustomerCityStateZip", "{0}");
    purchaseOrderReport.xrCustomerPhone.DataBindings.Add("Text", null, "PurchaseOrders.CustomerPhone", "{0}");
    //Items
    purchaseOrderReport.xrTableCell4.DataBindings.Add("Text", null, "PurchaseOrders.ModelNumber", "{0}");
    purchaseOrderReport.xrTableCell5.DataBindings.Add("Text", null, "PurchaseOrders.Quantity", "{0}");
    purchaseOrderReport.xrTableCell8.DataBindings.Add("Text", null, "PurchaseOrders.Price", "{0}");
    purchaseOrderReport.xrTableCell6.DataBindings.Add("Text", null, "PurchaseOrders.Description", "{0}");
    //LiftGate
    //   purchaseOrderReport.xrLiftGate.DataBindings.Add("Text", null, "PurchaseOrders.LiftGate", "{0}");

    DataTable dt = new DataTable();
    Dal.GetDataTable(dt, "SELECT TOP 1  LiftGate, UPSAccount, LocalPickUp from Purchase_Order where  PONumber='" + poNumber + "'");
    foreach (DataRow row in dt.Rows)
    {
        if ((bool)row["UPSAccount"])
        {
            purchaseOrderReport.xrUPS.Visible = true;
        }
        if ((bool)row["LocalPickUp"])
        {
            purchaseOrderReport.xrPickedUp.Visible = true;
        }
        if ((bool)row["LiftGate"])
        {
            purchaseOrderReport.xrLiftGate.Text = "YES";
        }
        else
        {
            purchaseOrderReport.xrLiftGate.Text = "NO";
        }
    }
    ReportPrintTool printTool = new ReportPrintTool(purchaseOrderReport);
    printTool.ShowRibbonPreview();   //Preview a Report
}

Down here at the bottom of the code is the actual code use to print preview the report. I also have another method same as the one above but its function is to print the report.

The problem with my code is when I need to change something I need to change it on my print method as well which breaks the law of OOP. Anyone here who's willing to guide me write this one into a better, more maintainable piece of code.

By the way, this is the code for my Print method

  public static void PrintPDF(string poNumber)
    {
        #region PrintPDF

        DataSet ds = new DataSet("PurchaseOrders");
        Dal.GetDataSet(ds, sqlQuery(poNumber));
        PurchaseReport purchaseOrderReport = new PurchaseReport
        {
            DataSource = ds,
            DataMember = "PurchaseOrders"
        };

        purchaseOrderReport.xrDateCreated.DataBindings.Add("Text", null, "PurchaseOrders.DateCreated",
            "{0:dddd, d MMMM, yyyy}");
        purchaseOrderReport.xrPO.DataBindings.Add("Text", null, "PurchaseOrders.PONumber", "{0}");
        purchaseOrderReport.xrVendor.DataBindings.Add("Text", null, "PurchaseOrders.POVendor", "{0}");
        purchaseOrderReport.xrPromocode.DataBindings.Add("Text", null, "PurchaseOrders.PromoCode", "{0}");
        //Bill Group
        //purchaseOrderReport.xrBusinessName.DataBindings.Add("Text", null, "PurchaseOrders.b_BillToGroup", "{0}");
        purchaseOrderReport.xrAddress1.DataBindings.Add("Text", null, "PurchaseOrders.b_AddressLine1", "{0}");
        purchaseOrderReport.xrAddress2.DataBindings.Add("Text", null, "PurchaseOrders.b_AddressLine2", "{0}");
        purchaseOrderReport.xrCityStateZip.DataBindings.Add("Text", null, "PurchaseOrders.CityStateZip", "{0}");
        purchaseOrderReport.xrPhone.DataBindings.Add("Text", null, "PurchaseOrders.b_Phone", "{0}");
        //Customer
        purchaseOrderReport.xrCustomerBusinessName.DataBindings.Add("Text", null,
            "PurchaseOrders.CustomerBusinessName", "{0}");
        purchaseOrderReport.xrCustomerName.DataBindings.Add("Text", null, "PurchaseOrders.CustomerName", "{0}");
        purchaseOrderReport.xrCustomerAddress.DataBindings.Add("Text", null, "PurchaseOrders.customerAddress", "{0}");
        purchaseOrderReport.xrCustomerCityStateZip.DataBindings.Add("Text", null,
            "PurchaseOrders.CustomerCityStateZip", "{0}");
        purchaseOrderReport.xrCustomerPhone.DataBindings.Add("Text", null, "PurchaseOrders.CustomerPhone", "{0}");
        //Items
        purchaseOrderReport.xrTableCell4.DataBindings.Add("Text", null, "PurchaseOrders.ModelNumber", "{0}");
        purchaseOrderReport.xrTableCell5.DataBindings.Add("Text", null, "PurchaseOrders.Quantity", "{0}");
        purchaseOrderReport.xrTableCell8.DataBindings.Add("Text", null, "PurchaseOrders.Price", "{0}");
        purchaseOrderReport.xrTableCell6.DataBindings.Add("Text", null, "PurchaseOrders.Description", "{0}");
        //LiftGate
        //   purchaseOrderReport.xrLiftGate.DataBindings.Add("Text", null, "PurchaseOrders.LiftGate", "{0}");

        DataTable dt = new DataTable();
        Dal.GetDataTable(dt,
            "SELECT TOP 1  LiftGate, UPSAccount, LocalPickUp from Purchase_Order where  PONumber='" + poNumber + "'");
        foreach (DataRow row in dt.Rows)
        {
            if ((bool) row["UPSAccount"])
            {
                purchaseOrderReport.xrUPS.Visible = true;
            }
            if ((bool) row["LocalPickUp"])
            {
                purchaseOrderReport.xrPickedUp.Visible = true;
            }
            if ((bool) row["LiftGate"])
            {
                purchaseOrderReport.xrLiftGate.Text = "YES";
            }
            else
            {
                purchaseOrderReport.xrLiftGate.Text = "NO";
            }
        }
        ReportPrintTool printTool = new ReportPrintTool(purchaseOrderReport);
        printTool.Print();

        #endregion
    }

And this is my code for the querying the database to get the data..

 private static string sqlQuery(string poNumber)
    {
        string sql= @"SELECT  a.POVendor ,
                        a.PONumber ,
                        a.DateCreated ,
                        a.PromoCode ,
                        a.CustomerBusinessName ,
                        a.CustomerName ,                           
                        CONCAT(a.CustomerAddress1, ' ', a.CustomerAddress2) AS customerAddress,
                        CONCAT(a.CustomerCity, ' ', a.CustomerState, ' ', a.CustomerZip) AS CustomerCityStateZip,                          
                        a.CustomerPhone ,
                        a.b_BillToGroup ,
                        a.b_AddressLine1 ,
                        a.b_AddressLine2 ,
                        a.b_City, 
                        a.b_State,
                        a.b_Zip,
                        CONCAT(a.b_City, ' ', a.b_State, ' ', a.b_Zip) as CityStateZip ,
                        a.b_Phone ,
                        b.ModelNumber ,
                        b.Quantity ,
                        b.Description,
                        '$' + Cast(b.Price as varchar(10)) as Price
                FROM    PURCHASE_ORDER a
                        INNER JOIN PURCHASE_ORDER_ITEMS b ON a.PONumber = b.PurchaseOrderItems                            
                WHERE   a.PONumber='" + poNumber + "'";
        return sql;
    }

This report is created using DevExpress Report, just in case you need other information.

Improve this question
\$\endgroup\$
4
  • \$\begingroup\$ If the poNumber in your code is user-provided I'd like to suggest you use a prepared statement. In the case of MySQL it would be something like MySqlCommand cmd = new MySqlCommand("SELECT ... FROM ... WHERE a.PONumber = ?poNumber", connection); cmd.Parameters.Add(new MySqlParameter("?poNumber", poNumber); \$\endgroup\$
    – vonludi
    Aug 19, 2016 at 6:15
  • \$\begingroup\$ Thanks for the concern but my poNumber is not user-provided.. \$\endgroup\$
    – Derstine
    Aug 19, 2016 at 10:37
  • \$\begingroup\$ @Derstine even if you're not worried about injection, it's worthwhile to use a prepared statement for the performance benefits. Idk about MySQL, but older versions of SQL Server would generate a new query plan for each poNumber it ever gets passed. Prepared statements allow it to use the plan cache instead. \$\endgroup\$
    – RubberDuck
    Aug 20, 2016 at 1:55
  • \$\begingroup\$ @RubberDuck Thanks for the input. I didn't know about that. \$\endgroup\$
    – Derstine
    Aug 21, 2016 at 3:07

1 Answer 1

Reset to default
2
\$\begingroup\$

Only the most evident flaws ...

  1. Very probably your code is vulnerable to SQL injection attacks ...
  2. If you want OOP suggestions then i could say that Single Responsibility Principle is violated. Your method CreateReportResult (not even a class, but a method) does a few things - retrieve data from database and create report. So move this logic into DAL "SELECT TOP 1 LiftGate, UPSAccount, LocalPickUp from Purchase_Order where PONumber='" + poNumber + "'");

  3. Move duplicated code into separate method (as you mentioned for new requests\improvements you have to change code in 2 methods). Like this

    public static void CreateReportResult(string poNumber)
{
    var printTool = GetReportPrintTool();
    printTool.ShowRibbonPreview();   //Preview a Report
}
public static void PrintPDF(string poNumber)
{
    ReportPrintTool printTool = GetReportPrintTool();
    printTool.Print();
}

public static ReportPrintTool GetReportPrintTool()
{
    DataSet ds = new DataSet("PurchaseOrders");
    Dal.GetDataSet(ds, sqlQuery(poNumber));
    PurchaseReport purchaseOrderReport = new PurchaseReport
    {
        DataSource = ds,
        DataMember = "PurchaseOrders"
    };


    purchaseOrderReport.xrDateCreated.DataBindings.Add("Text", null, "PurchaseOrders.DateCreated", "{0:dddd, d MMMM, yyyy}");
    purchaseOrderReport.xrPO.DataBindings.Add("Text", null, "PurchaseOrders.PONumber", "{0}");
    purchaseOrderReport.xrVendor.DataBindings.Add("Text", null, "PurchaseOrders.POVendor", "{0}");
    purchaseOrderReport.xrPromocode.DataBindings.Add("Text", null, "PurchaseOrders.PromoCode", "{0}");
    //Bill Group
    //purchaseOrderReport.xrBusinessName.DataBindings.Add("Text", null, "PurchaseOrders.b_BillToGroup", "{0}");
    purchaseOrderReport.xrAddress1.DataBindings.Add("Text", null, "PurchaseOrders.b_AddressLine1", "{0}");
    purchaseOrderReport.xrAddress2.DataBindings.Add("Text", null, "PurchaseOrders.b_AddressLine2", "{0}");
    purchaseOrderReport.xrCityStateZip.DataBindings.Add("Text", null, "PurchaseOrders.CityStateZip", "{0}");
    purchaseOrderReport.xrPhone.DataBindings.Add("Text", null, "PurchaseOrders.b_Phone", "{0}");
    //Customer
    purchaseOrderReport.xrCustomerBusinessName.DataBindings.Add("Text", null, "PurchaseOrders.CustomerBusinessName", "{0}");
    purchaseOrderReport.xrCustomerName.DataBindings.Add("Text", null, "PurchaseOrders.CustomerName", "{0}");
    purchaseOrderReport.xrCustomerAddress.DataBindings.Add("Text", null, "PurchaseOrders.customerAddress", "{0}");
    purchaseOrderReport.xrCustomerCityStateZip.DataBindings.Add("Text", null, "PurchaseOrders.CustomerCityStateZip", "{0}");
    purchaseOrderReport.xrCustomerPhone.DataBindings.Add("Text", null, "PurchaseOrders.CustomerPhone", "{0}");
    //Items
    purchaseOrderReport.xrTableCell4.DataBindings.Add("Text", null, "PurchaseOrders.ModelNumber", "{0}");
    purchaseOrderReport.xrTableCell5.DataBindings.Add("Text", null, "PurchaseOrders.Quantity", "{0}");
    purchaseOrderReport.xrTableCell8.DataBindings.Add("Text", null, "PurchaseOrders.Price", "{0}");
    purchaseOrderReport.xrTableCell6.DataBindings.Add("Text", null, "PurchaseOrders.Description", "{0}");
    //LiftGate
    //   purchaseOrderReport.xrLiftGate.DataBindings.Add("Text", null, "PurchaseOrders.LiftGate", "{0}");

    DataTable dt = new DataTable();
    Dal.GetDataTable(dt, "SELECT TOP 1  LiftGate, UPSAccount, LocalPickUp from Purchase_Order where  PONumber='" + poNumber + "'");
    foreach (DataRow row in dt.Rows)
    {
        if ((bool)row["UPSAccount"])
        {
            purchaseOrderReport.xrUPS.Visible = true;
        }
        if ((bool)row["LocalPickUp"])
        {
            purchaseOrderReport.xrPickedUp.Visible = true;
        }
        if ((bool)row["LiftGate"])
        {
            purchaseOrderReport.xrLiftGate.Text = "YES";
        }
        else
        {
            purchaseOrderReport.xrLiftGate.Text = "NO";
        }
    }
    ReportPrintTool printTool = new ReportPrintTool(purchaseOrderReport);

    return printTool;
}
Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much. I will try to rewrite my code like you suggested. \$\endgroup\$
    – Derstine
    Aug 19, 2016 at 10:40

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.