2
\$\begingroup\$

I'm creating a simple windows form application containing a combo box, a text box, a datagridview, a button and some radio buttons. The idea is upon the form loading the combo box is to be populated with the data of a specific column ("Party") of a source excel file. Then when a valid item is selected from the combo box, the excel file data is to be filtered and shown in the datagridview. Further filtering can be done by entering some text to the text box and then hitting a button.

Please find below my code to understand it better:

    void DataFormLoad(object sender, EventArgs e)
    {
        ExcelPackage.LicenseContext =LicenseContext.NonCommercial;
        int lastRow = 0;
        
        using (ExcelPackage package = new ExcelPackage(new System.IO.FileInfo(@"database.xlsx"), false))
        {
            ExcelWorksheet mainSheet = package.Workbook.Worksheets.First();

            for (int i = 2; i <= mainSheet.Dimension.End.Row; i++)
            {
                if (!string.IsNullOrEmpty(mainSheet.Cells["A"+i].Text))
                {
                    lastRow =i;
                }
            }
            List<string> party = new List<string>();
            
            for (int row = 2; row <= lastRow; row++)
            {
                if (!string.IsNullOrEmpty(mainSheet.Cells[row, 1].Text))
                {
                    party.Add(mainSheet.Cells[row, 1].Text);
                }
            }
            
            
            foreach (var element in party.Distinct())
            {
                kryptonComboBox1.Items.Add(element);
            }

        }
        
        kryptonLabel2.Text = "Enter full invoice number or a part of it";

    }
    void KryptonComboBox1SelectedIndexChanged(object sender, EventArgs e)
    {

        kryptonDataGridView1.DataSource=null;
        kryptonTextBox1.Text="";
        
        int rowno = 0;
        
        string kword = kryptonComboBox1.Text;

        
        using (ExcelPackage package = new ExcelPackage(new System.IO.FileInfo(@"database.xlsx"), false))
        {
            ExcelWorksheet workSheet = package.Workbook.Worksheets[0];

            for (int i = 2; i <= workSheet.Dimension.End.Row; i++)
            {
                if (!string.IsNullOrEmpty(workSheet.Cells["A"+i].Text))
                {
                    rowno =i;
                }
            }
            
            DataTable dtTemp = new DataTable();
            
            dtTemp.Columns.Add("Party");
            dtTemp.Columns.Add("Bill No.");
            dtTemp.Columns.Add("Bill Date");
            dtTemp.Columns.Add("Amount");
            dtTemp.Columns.Add("Due Date");
            dtTemp.Columns.Add("Remarks");
            dtTemp.Columns.Add("Payment Released on");
            
            DataRow drAddItem;
            for (int row = 2; row <= rowno; row++)
            {
                
                if (workSheet.Cells[row, 1].Text == kword)
                {
                    
                    object col1Value = workSheet.Cells[row, 1].Text;
                    object col2Value = workSheet.Cells[row, 2].Text;
                    object col3Value = workSheet.Cells[row, 3].Text;
                    object col4Value = workSheet.Cells[row, 4].Text;
                    object col5Value = workSheet.Cells[row, 5].Text;
                    object col6Value = workSheet.Cells[row, 6].Text;
                    object col8Value = workSheet.Cells[row, 8].Text;

                    drAddItem = dtTemp.NewRow();

                    drAddItem["Party"] = col1Value.ToString();
                    drAddItem["Bill No."] = col2Value.ToString();
                    drAddItem["Bill Date"] = col3Value.ToString();
                    drAddItem["Amount"] = col4Value.ToString();
                    drAddItem["Due Date"] = col5Value.ToString();
                    drAddItem["Remarks"] = col6Value.ToString();
                    drAddItem["Payment Released on"] = col8Value.ToString();
                    
                    dtTemp.Rows.Add(drAddItem);
                    
                    
                }
            }
            kryptonDataGridView1.DataSource=dtTemp;
        }
        

    }


    void KryptonComboBox1Leave(object sender, EventArgs e)
    {
        var indx = kryptonComboBox1.FindStringExact(kryptonComboBox1.Text);
        if(indx <= -1)
        {
            MetroMessageBox.Show(this,"Please type/select a valid Party Name","Invalid Party Name",MessageBoxButtons.OK,MessageBoxIcon.Error);
            kryptonComboBox1.Text="";
            return;
        }
        
    }


    void KryptonButton1Click(object sender, EventArgs e)
    {
        kryptonDataGridView1.DataSource=null;
        
        int rowno = 0;
        
        string kword = kryptonComboBox1.Text;
        string kword2 = kryptonTextBox1.Text;
        double kw;

        
        using (ExcelPackage package = new ExcelPackage(new System.IO.FileInfo(@"database.xlsx"), false))
        {
            ExcelWorksheet workSheet = package.Workbook.Worksheets[0];

            for (int i = 2; i <= workSheet.Dimension.End.Row; i++)
            {
                if (!string.IsNullOrEmpty(workSheet.Cells["A"+i].Text))
                {
                    rowno =i;
                }
            }
            
            DataTable dtTemp = new DataTable();
            
            dtTemp.Columns.Add("Party");
            dtTemp.Columns.Add("Bill No.");
            dtTemp.Columns.Add("Bill Date");
            dtTemp.Columns.Add("Amount");
            dtTemp.Columns.Add("Due Date");
            dtTemp.Columns.Add("Remarks");
            dtTemp.Columns.Add("Payment Released on");
            
            DataRow drAddItem;
            for (int row = 2; row <= rowno; row++)
            {
                
                if (kryptonRadioButton1.Checked)
                {
                    if (workSheet.Cells[row, 1].Text == kword && workSheet.Cells[row, 2].Text.ToUpper().Contains(kword2.ToUpper()))
                    {

                        object col1Value = workSheet.Cells[row, 1].Text;
                        object col2Value = workSheet.Cells[row, 2].Text;
                        object col3Value = workSheet.Cells[row, 3].Text;
                        object col4Value = workSheet.Cells[row, 4].Text;
                        object col5Value = workSheet.Cells[row, 5].Text;
                        object col6Value = workSheet.Cells[row, 6].Text;
                        object col8Value = workSheet.Cells[row, 8].Text;

                        drAddItem = dtTemp.NewRow();

                        drAddItem["Party"] = col1Value.ToString();
                        drAddItem["Bill No."] = col2Value.ToString();
                        drAddItem["Bill Date"] = col3Value.ToString();
                        drAddItem["Amount"] = col4Value.ToString();
                        drAddItem["Due Date"] = col5Value.ToString();
                        drAddItem["Remarks"] = col6Value.ToString();
                        drAddItem["Payment Released on"] = col8Value.ToString();
                        
                        dtTemp.Rows.Add(drAddItem);
                        
                    }
                }
                
                if (kryptonRadioButton2.Checked)
                {
                    
                    try
                    {
                        Convert.ToDateTime(kword2).ToString("dd-MM-yyyy");
                    }

                    catch
                    {
                        MessageBox.Show("Please enter a valid date!!\n");
                        kryptonTextBox1.Text="";
                        break;
                    }
                    
                    if (workSheet.Cells[row, 1].Text == kword && workSheet.Cells[row, 3].Text == Convert.ToDateTime(kword2).ToString("dd-MM-yyyy"))
                    {

                        object col1Value = workSheet.Cells[row, 1].Text;
                        object col2Value = workSheet.Cells[row, 2].Text;
                        object col3Value = workSheet.Cells[row, 3].Text;
                        object col4Value = workSheet.Cells[row, 4].Text;
                        object col5Value = workSheet.Cells[row, 5].Text;
                        object col6Value = workSheet.Cells[row, 6].Text;
                        object col8Value = workSheet.Cells[row, 8].Text;

                        drAddItem = dtTemp.NewRow();

                        drAddItem["Party"] = col1Value.ToString();
                        drAddItem["Bill No."] = col2Value.ToString();
                        drAddItem["Bill Date"] = col3Value.ToString();
                        drAddItem["Amount"] = col4Value.ToString();
                        drAddItem["Due Date"] = col5Value.ToString();
                        drAddItem["Remarks"] = col6Value.ToString();
                        drAddItem["Payment Released on"] = col8Value.ToString();
                        
                        dtTemp.Rows.Add(drAddItem);
                        
                    }
                }
                
                if (kryptonRadioButton3.Checked)
                {
                    if (double.TryParse(kword2,out kw))
                    {

                        if (workSheet.Cells[row, 1].Text == kword && Convert.ToDouble(workSheet.Cells[row, 4].Text).ToString() == Math.Round(kw,2).ToString())
                        {

                            object col1Value = workSheet.Cells[row, 1].Text;
                            object col2Value = workSheet.Cells[row, 2].Text;
                            object col3Value = workSheet.Cells[row, 3].Text;
                            object col4Value = workSheet.Cells[row, 4].Text;
                            object col5Value = workSheet.Cells[row, 5].Text;
                            object col6Value = workSheet.Cells[row, 6].Text;
                            object col8Value = workSheet.Cells[row, 8].Text;

                            drAddItem = dtTemp.NewRow();

                            drAddItem["Party"] = col1Value.ToString();
                            drAddItem["Bill No."] = col2Value.ToString();
                            drAddItem["Bill Date"] = col3Value.ToString();
                            drAddItem["Amount"] = col4Value.ToString();
                            drAddItem["Due Date"] = col5Value.ToString();
                            drAddItem["Remarks"] = col6Value.ToString();
                            drAddItem["Payment Released on"] = col8Value.ToString();
                            
                            dtTemp.Rows.Add(drAddItem);
                            
                        }
                        
                    }
                    else
                        
                    {
                        MessageBox.Show("Please enter a numeric value");
                        break;
                    }
                }
            }
            kryptonDataGridView1.DataSource=dtTemp;
        }
        
    }
    void KryptonRadioButton2CheckedChanged(object sender, EventArgs e)
    {
        kryptonTextBox1.Text="";
        kryptonTextBox1.Focus();
        kryptonLabel2.Text = "Enter a complete date (day, then month and then year)";
    }
    void KryptonRadioButton3CheckedChanged(object sender, EventArgs e)
    {
        kryptonTextBox1.Text="";
        kryptonTextBox1.Focus();
        kryptonLabel2.Text = "Enter exact amount as per Invoice";
    }
    void KryptonRadioButton1CheckedChanged(object sender, EventArgs e)
    {
        kryptonTextBox1.Text="";
        kryptonTextBox1.Focus();
        kryptonLabel2.Text = "Enter full invoice number or a part of it";
    }

The code is very redundant, specially the portion

                            object col1Value = workSheet.Cells[row, 1].Text;
                            object col2Value = workSheet.Cells[row, 2].Text;
                            object col3Value = workSheet.Cells[row, 3].Text;
                            object col4Value = workSheet.Cells[row, 4].Text;
                            object col5Value = workSheet.Cells[row, 5].Text;
                            object col6Value = workSheet.Cells[row, 6].Text;
                            object col8Value = workSheet.Cells[row, 8].Text;

                            drAddItem = dtTemp.NewRow();

                            drAddItem["Party"] = col1Value.ToString();
                            drAddItem["Bill No."] = col2Value.ToString();
                            drAddItem["Bill Date"] = col3Value.ToString();
                            drAddItem["Amount"] = col4Value.ToString();
                            drAddItem["Due Date"] = col5Value.ToString();
                            drAddItem["Remarks"] = col6Value.ToString();
                            drAddItem["Payment Released on"] = col8Value.ToString();
                            
                            dtTemp.Rows.Add(drAddItem);

How can I make it less redundant and efficient? What other changes can be made to make the code faster if possible?

The app UI looks like this app

The excel looks like sample.xlsx

\$\endgroup\$
4
\$\begingroup\$

In order to reduce repetitive code you need to understand which steps of actions you are repeating

//Retrieving column values for a given row from 1 to 8 except 7
object col1Value = workSheet.Cells[row, 1].Text;
object col2Value = workSheet.Cells[row, 2].Text;
object col3Value = workSheet.Cells[row, 3].Text;
object col4Value = workSheet.Cells[row, 4].Text;
object col5Value = workSheet.Cells[row, 5].Text;
object col6Value = workSheet.Cells[row, 6].Text;
object col8Value = workSheet.Cells[row, 8].Text;

drAddItem = dtTemp.NewRow();

//Populating the retrieved values into the related cells of the DataRow 
drAddItem["Party"] = col1Value.ToString();
drAddItem["Bill No."] = col2Value.ToString();
drAddItem["Bill Date"] = col3Value.ToString();
drAddItem["Amount"] = col4Value.ToString();
drAddItem["Due Date"] = col5Value.ToString();
drAddItem["Remarks"] = col6Value.ToString();
drAddItem["Payment Released on"] = col8Value.ToString();

dtTemp.Rows.Add(drAddItem);

The retrieving logic can be simplified really easily by using a loop

for(int column = 1; column < 9; column++)
{
    if (column == 7) continue;
    object columnValue = workSheet.Cells[row, column].Text;
}

The mapping logic can be simplified by utilizing an array to preserve the ordering

var columnMapping = new [] 
{ 
    "Party",
    "Bill No.",
    "Bill Date",
    "Amount", 
    "Due Date",
    "Remarks",  
    "Payment Released on" 
};

Now you need to combine these together

var columnMapping = new[]
{
    "Party",
    "Bill No.",
    "Bill Date",
    "Amount",
    "Due Date",
    "Remarks",  
    "Payment Released on"
};

drAddItem = dtTemp.NewRow();
dtTemp.Rows.Add(drAddItem);
var columnIterator = columnMapping.GetEnumerator();
for (int column = 1; column < 9; column++)
{
    if (column == 7) continue;
    columnIterator.MoveNext();
    
    object columnValue = workSheet.Cells[row, column].Text;
    drAddItem[columnIterator.Current.ToString()] = columnValue.ToString();
}

I would like to warn you that this code is fragile since the column indexer and the column mapping are living separately. It means if you modify one of them you have to remember to modify the other one as well. There will be no compile error that you have modified one but you did not update the other.


UPDATE Populating columnMapping from dtTemp's Columns

You can retrieve the column names from the DataTable and use that to populate the mappings in the following way:

var dtTemp = new DataTable();
dtTemp.Columns.AddRange(new[]
{
    new DataColumn("Party"),
    new DataColumn("Bill No."),
    new DataColumn("Bill Date"),
    new DataColumn("Amount"),
    new DataColumn("Due Date"),
    new DataColumn("Remarks"),
    new DataColumn("Payment Released on"),
});

var columnMapping = dtTemp.Columns
    .Cast<DataColumn>()
    .Select(col => col.ColumnName);

Because Columns is a DataColumnCollection you can't issue Linq queries directly against it you need to Cast it first.

\$\endgroup\$
14
  • \$\begingroup\$ I'm not sure what you meant by "this code is fragile since the column indexer and the column mapping are living separately. It means if you modify one of them you have to remember to modify the other one as well"....I'm only going to read data from the excel file.. \$\endgroup\$ Jan 13 at 11:24
  • \$\begingroup\$ @TamalBanerjee My proposed solution is fragile in a way that let's suppose you have one more column that you want to add. You change the for loop's exit condition from column < 9; to column < 10; but forgot to add a new mapping inside the columnMapping array. The code compiles, but the data will not appear on screen. Is it clear now? \$\endgroup\$ Jan 13 at 11:45
  • 1
    \$\begingroup\$ understood... Thanks for your answer. I'll let you know if I have further queries regarding your code. \$\endgroup\$ Jan 13 at 11:54
  • 1
    \$\begingroup\$ @TamalBanerjee Please do not simply copy-paste code and then run to here whenever something doesn't work. Being a coder means you need to understand what you are doing: do not just copy-paste the code, understand what that code does and how it does it. IMHO code in code reviews can be "imperfect" in case it is used to simply demonstrate a concept, it is a guideline not a fully-fledged solution. Learn from the example and use your newly acquired knowledge to improve not just this bit of code, but all code you write. \$\endgroup\$
    – BCdotWEB
    Jan 14 at 10:09
  • 1
    \$\begingroup\$ @PeterCsala Thanks for your help... I figured it out \$\endgroup\$ Jan 14 at 12:49
0
\$\begingroup\$

You have already identified repeated code. You should probably make that code block into a function that accepts a DataTable, a ExcelWorksheet and a row number.

\$\endgroup\$
6
  • \$\begingroup\$ Any other thing that could be better? Can the radio button checking be handled better? Thanks in advance \$\endgroup\$ Jan 13 at 9:11
  • \$\begingroup\$ No idea - I'll leave those questions for other reviewers (i.e. those who actually know the language and the library). \$\endgroup\$ Jan 13 at 10:24
  • 1
    \$\begingroup\$ I don't want to be rude but this post is more like a comment than an answer. \$\endgroup\$ Jan 13 at 10:38
  • \$\begingroup\$ @Peter, it does identify an improvement that can be made to the code. It's a short answer, but definitely an answer. \$\endgroup\$ Jan 13 at 12:09
  • 1
    \$\begingroup\$ (And yes, I have seen short answers like that written in comments - but I don't think they belong there). \$\endgroup\$ Jan 13 at 12:10

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.