4
\$\begingroup\$

This checks each row for a category. It stores ONE version of the category, ignore duplications. It then counts how many times a category appears.

There are a lot of for loops and a lot of table-checking. This takes time as the table is quite large. If it's possible, I would like to incorporate some of the loops to reduce time spent.

public partial class Stats : Form
{
    public Stats(ref TabControl languageTabs)
    {
        InitializeComponent();

        for (int i = 0; i < languageTabs.TabCount; i++)
        {
            Control[] ctrls = languageTabs.TabPages[i].Controls.Find(languageTabs.TabPages[i].Name + "Grid", true);
            DataGridView dgv = ctrls[0] as DataGridView;

            int charCount = 0;
            for (int j = 0; j < dgv.RowCount; j++)
            {
                charCount += (int)dgv.Rows[j].Cells[4].Value;
            }

            treeView1.Nodes.Add(languageTabs.TabPages[i].Text.ToUpper() + " Statistics");
            treeView1.Nodes[i].Nodes.Add("Total Number Of Strings: " + dgv.RowCount); // might need to remove '--' from count
            treeView1.Nodes[i].Nodes.Add("Total Number Of Chars: " + charCount);
            treeView1.Nodes[i].Nodes.Add("Total Number Of Strings and Chars per Category: ");

            //check for all different categories
            List<string> categories = new List<string>();

            int count = 0;
            for (int j = 0; j < dgv.RowCount; j++)
            {
                // look at every category in the dgv and see if we have a unique value in a container
                List<string> result = categories.FindAll(
                    delegate(string b)
                    {
                        return b == dgv.Rows[j].Cells[1].Value.ToString();
                    }
                );

                // if we don't have a unique ID then add it
                if (result.Count == 0)
                {
                    categories.Add(dgv.Rows[j].Cells[1].Value.ToString());
                }  
            }

            int[] categoryCount = new int[categories.Count];

            foreach (string cat in categories)
            {
                for (int j = 0; j < dgv.RowCount; j++)
                {
                    if (dgv.Rows[j].Cells[1].Value.ToString() == cat)
                    {
                        categoryCount[count]++;
                    }
                }
                count++;
            }

            int countTwo = 0;
            foreach (string cat in categories)
            {
                treeView1.Nodes[i].Nodes[2].Nodes.Add(cat + ":   " + categoryCount[countTwo].ToString());
                countTwo++;
            }

        }
    }
\$\endgroup\$
1
  • \$\begingroup\$ You can always user recursion to remove for-loops. Also, while loops or do-while loops will suffice too. \$\endgroup\$ Dec 7, 2011 at 21:21

4 Answers 4

5
\$\begingroup\$

There are two pairs of loops that are the same and can be combined like this:

int charCount = 0;
List<string> categories = new List<string>();
for (int j = 0; j < dgv.RowCount; j++)
{
    charCount += (int)dgv.Rows[j].Cells[4].Value;

    // look at every category in the dgv and see if we have a unique value in a container
    List<string> result = categories.FindAll(
        delegate(string b)
        {
            return b == dgv.Rows[j].Cells[1].Value.ToString();
        }
    );
    // if we don't have a unique ID then add it
    if (result.Count == 0)
    {
        categories.Add(dgv.Rows[j].Cells[1].Value.ToString());
    }
}

treeView1.Nodes.Add(languageTabs.TabPages[i].Text.ToUpper() + " Statistics");
treeView1.Nodes[i].Nodes.Add("Total Number Of Strings: " + dgv.RowCount); // might need to remove '--' from count
treeView1.Nodes[i].Nodes.Add("Total Number Of Chars: " + charCount);
treeView1.Nodes[i].Nodes.Add("Total Number Of Strings and Chars per Category: ");

int[] categoryCount = new int[categories.Count];
int count = 0;
foreach (string cat in categories)
{
    for (int j = 0; j < dgv.RowCount; j++)
    {
        if (dgv.Rows[j].Cells[1].Value.ToString() == cat)
        {
            categoryCount[count]++;
        }
    }

    treeView1.Nodes[i].Nodes[2].Nodes.Add(cat + ":   " + categoryCount[count].ToString());
    count++;
}

Note that the categoryCount array is no longer ncessary, and add Saeed's answer and you get:

int charCount = 0;
List<string> categories = new List<string>();
for (int j = 0; j < dgv.RowCount; j++)
{
    charCount += (int)dgv.Rows[j].Cells[4].Value;

    if (!categories.Any(x=>x == dgv.Rows[j].Cells[1].Value.ToString()))
        categories.Add(dgv.Rows[j].Cells[1].Value.ToString()); 
}

treeView1.Nodes.Add(languageTabs.TabPages[i].Text.ToUpper() + " Statistics");
treeView1.Nodes[i].Nodes.Add("Total Number Of Strings: " + dgv.RowCount); // might need to remove '--' from count
treeView1.Nodes[i].Nodes.Add("Total Number Of Chars: " + charCount);
treeView1.Nodes[i].Nodes.Add("Total Number Of Strings and Chars per Category: ");

foreach (string cat in categories)
{
    int count = 0;
    for (int j = 0; j < dgv.RowCount; j++)
    {
        if (dgv.Rows[j].Cells[1].Value.ToString() == cat)
        {
            count++;
        }
    }

    treeView1.Nodes[i].Nodes[2].Nodes.Add(cat + ":   " + count.ToString());
}

Then, assuming that dgv.Rows implements IEnumerable you can do this to the second loop:

foreach (string cat in categories)
{
    int count = dgv.Rows.Where(r => r.Cells[1].Value.ToString() == cat).Count();
    treeView1.Nodes[i].Nodes[2].Nodes.Add(cat + ":   " + count.ToString());
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ @MatthewLowe - if you link your accounts when you get 200 on one site you'll get a 100 point bonus across all sites giving you a head start on the other sites. \$\endgroup\$
    – user3040
    Dec 7, 2011 at 12:43
  • \$\begingroup\$ @ChrisF -- Thanks. I'll start saving my points :) \$\endgroup\$ Dec 7, 2011 at 12:45
4
\$\begingroup\$

Besides the already mentioned points, you can also improve the code by storing duplicate 'object paths' in temporary intermediate variables.

E.g. you can easily change the following code segment from ...

treeView1.Nodes.Add(languageTabs.TabPages[i].Text.ToUpper() + " Statistics");
treeView1.Nodes[i].Nodes.Add("Total Number Of Strings: " + dgv.RowCount); // might need to remove '--' from count
treeView1.Nodes[i].Nodes.Add("Total Number Of Chars: " + charCount);
treeView1.Nodes[i].Nodes.Add("Total Number Of Strings and Chars per Category: ");

... to:

treeView1.Nodes.Add(languageTabs.TabPages[i].Text.ToUpper() + " Statistics");
var nodes = treeView1.Nodes[i].Nodes;
nodes.Add("Total Number Of Strings: " + dgv.RowCount); // might need to remove '--' from count
nodes.Add("Total Number Of Chars: " + charCount);
nodes.Add("Total Number Of Strings and Chars per Category: ");
\$\endgroup\$
0
3
\$\begingroup\$

You can reduce some parts like:

        for (int j = 0; j < dgv.RowCount; j++)
        {
            // look at every category in the dgv and see if 
            // we have a unique value in a container
            List<string> result = categories.FindAll(
                delegate(string b)
                {
                    return b == dgv.Rows[j].Cells[1].Value.ToString();
                }
            );

            // if we don't have a unique ID then add it
            if (result.Count == 0)
            {
                categories.Add(dgv.Rows[j].Cells[1].Value.ToString());
            }  
        }

To:

        for (int j = 0; j < dgv.RowCount; j++)
        {
            // look at every category in the dgv and see if 
            // we have a unique value in a container
            // if we don't have a unique ID then add it

            if (!categories.Any(x=>x == dgv.Rows[j].Cells[1].Value.ToString()))
                categories.Add(dgv.Rows[j].Cells[1].Value.ToString());

        }

After you edit it as this, all part of your code it will be more readable, and you can decide how to remove loops. In fact I suggest move from delegates to lambda expression, and use related linq function in each part, not just functions which solves your problems.

e.g In the case I wrote Any returns first time found any item, and it doesn't iterate over all data in categories.

\$\endgroup\$
0
\$\begingroup\$

If you have the underlying data sources of your grids handy, you could simplify much of that code by using LINQ grouping.

For example, if you had a System.Data.DataTable object populating your grids, a LINQ statement that could pull your category and character count information could be as follows:

 var  categories = from row in table.AsEnumerable ()
                   group row by row [1] into groupedRows
                   select new { Value = groupedRows.Key,
                                Count = groupedRows.Count (),
                                CharSum = groupedRows.Sum (r => r.Field<int>(4))};
 var  charTotal = categories.Sum (c => c.CharSum);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.