5
\$\begingroup\$

I am trying to get equal number of records in both columns, even if that means that first column doesn't fill page to bottom on the second page. The only way I was able to do this was by using a maxrow count and breaking when it is equal to the max row count.

 public List<DataForExcelExport> GetData()
        {

            var list = new List<DataForExcelExport>();
            int ROWS_PER_PAGE = 41;
            int MaxRow = 0;

            SqlDataAdapter dataAdapter = new SqlDataAdapter(selectCommandText, connectionString);
            DataTable dataTable = new DataTable();
            dataAdapter.Fill(dataTable);
            bool flgLastPage = false;
            for (int i = 0; i < dataTable.Rows.Count; i++)
            {
                DataRow dr = dataTable.Rows[i];
                var item = new DataForExcelExport();
                SetItemData(dr, item, true);
                if (i + ROWS_PER_PAGE < dataTable.Rows.Count)
                {
                    SetItemData(dataTable.Rows[i + ROWS_PER_PAGE], item, false);
                }
                list.Add(item);
                if (flgLastPage == false)
                {
                    if ((i + 1) % ROWS_PER_PAGE == 0)
                    {
                        i += ROWS_PER_PAGE;

                        int remaining = dataTable.Rows.Count - (i + 1);
                        if (remaining < 2 * ROWS_PER_PAGE)
                        {
                            ROWS_PER_PAGE = remaining / 2 + remaining % 2;
                            flgLastPage = true;

                        }
                    }
                }
                int maxrow = i + ROWS_PER_PAGE;
                if (maxrow == dataTable.Rows.Count)
                {
                    break;

                }
            }
            return list;

        }

expected output: when there are a total of 118 rows

- page 1 
_________
  1 | 41
... |...
 38 | 78
 39 | 79
 40 | 80
_________

- page 2
_________
 81 | 99
 82 | 100
... | ...
98  |118
_________
\$\endgroup\$
  • \$\begingroup\$ It is not easy to understand this code. I think there is too many variables changing their values while you are loading your data. My advice is - as you know dataTable.Rows.Count and ROWS_PER_PAGE you can calculate the number of pages, and number of rows on the given page in advance, then process things with 2 loops. One for pages, one for rows \$\endgroup\$ – jakubiszon Nov 1 at 15:57
  • \$\begingroup\$ This would be a lot easier if you posted the whole program. So am I correct in now assuming you want 80 entries per page? \$\endgroup\$ – T145 Nov 4 at 20:57
  • \$\begingroup\$ Hey there, looks like you're having login issues; feel free to contact the community team if you would like your two accounts merged. \$\endgroup\$ – Mathieu Guindon Nov 4 at 21:24
  • \$\begingroup\$ @T145 correct page 1 should display 80 rows and then the remaining should display on page 2 \$\endgroup\$ – Jefferson Nov 5 at 15:22
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Nov 18 at 17:05
3
+25
\$\begingroup\$

Try following some simple rules: Avoid setting flags in loops. Try calculating the data you need before taking action. Only increment counters inside loop declaration. This way even if something gets broken - you will not have to wonder "how did we end up here?". Instead you will be able to tell "this index / limit is not calculated correctly".

Also when saying

even if that means that first column doesn't fill page to bottom on the second page

Did you mean:

even if that means both columns don't fill the last page to the bottom

This is how I understood it.

int ROWS_PER_PAGE = 41;
int totalDataCount = dataTable.Rows.Count;
int pagesToProcess = (int)Math.Ceiling(totalDataCount / 2.0 / ROWS_PER_PAGE);
int itemsPerPage = ROWS_PER_PAGE * 2;

for(int pageIdx = 0; pageIdx < pagesToProcess; pageIdx++)
{
    int itemsOnThisPage = (pageIdx + 1) * itemsPerPage >= totalDataCount ? totalDataCount - pageIdx * itemsPerPage : itemsPerPage;
    int rowsOnThisPage = (int)Math.Ceiling( itemsOnThisPage / 2.0 );
    for( int rowIdx = 0; rowIdx < rowsOnThisPage; rowIdx++)
    {
        var item = new DataForExcelExport();
        int itemIdx1 = pageIdx * itemsPerPage + rowIdx;
        int itemIdx2 = itemIdx1 + rowsOnThisPage;
        SetItemData(dataTable.Rows[itemIdx1], item, true);
        if(totalDataCount > itemIdx2)
            SetItemData(dataTable.Rows[itemIdx2], item, false);
    }
}

Ah yes, and try to name variables in a meaningful way. Names like i or maxrow do not tell what the variable is really used for.

\$\endgroup\$
  • \$\begingroup\$ I get an error about can't convert double to int on the second line \$\endgroup\$ – Jefferson Nov 1 at 20:04
  • \$\begingroup\$ On the Math.Ceiling? There might be a cast needed, try =(int)Math.Ceiling(...). \$\endgroup\$ – jakubiszon Nov 2 at 7:42
  • \$\begingroup\$ @jakubiszon I dont get out of the innter for loop? when do I add the item to the list? \$\endgroup\$ – Jefferson Nov 14 at 16:08
  • \$\begingroup\$ After processing page 1 it set itemsOnThisPage back to 82 and rowsOnThisPage to 41 should that just be the remaining because then itemIdx1 should be 42 it goes back to 0 \$\endgroup\$ – Jefferson Nov 14 at 16:29
  • \$\begingroup\$ 1) What do you mean by not getting out of the inner for loop? Not using break? No it is not needed. Break is used when you meet an unforseen condition. There is nothing unforseen to happen here. \$\endgroup\$ – jakubiszon Nov 15 at 17:23
1
\$\begingroup\$

Because this function is presented w/out a context to adequately test the code in, i.e. with sample data, a Main function, etc., the best way to critique this presently is against the design.

Let us apply some basic formatting so we can actually get a better idea of what's going on:

public List<DataForExcelExport> GetData()
{
    var list = new List<DataForExcelExport>();
    int ROWS_PER_PAGE = 41;
    int MaxRow = 0;

    var dataAdapter = new SqlDataAdapter(selectCommandText, connectionString);
    var dataTable = new DataTable();
    dataAdapter.Fill(dataTable);
    bool flgLastPage = false;

    for (int i = 0; i < dataTable.Rows.Count; i++)
    {
        DataRow dr = dataTable.Rows[i];
        var item = new DataForExcelExport();
        SetItemData(dr, item, true);

        if (i + ROWS_PER_PAGE < dataTable.Rows.Count)
        {
            SetItemData(dataTable.Rows[i + ROWS_PER_PAGE], item, false);
        }

        list.Add(item);

        if (flgLastPage == false)
        {
            if ((i + 1) % ROWS_PER_PAGE == 0)
            {
                i += ROWS_PER_PAGE;

                int remaining = dataTable.Rows.Count - (i + 1);
                if (remaining < 2 * ROWS_PER_PAGE)
                {
                    ROWS_PER_PAGE = remaining / 2 + remaining % 2;
                    flgLastPage = true;
                }
            }
        }

        int maxrow = i + ROWS_PER_PAGE;

        if (maxrow == dataTable.Rows.Count)
        {
            break;
        }
    }

    return list;
}

Ah, that's better. Now, to me, the orphaned variables selectCommandText and connectionString appear as though they're global constants since they are not parameters. If they are, they should look like COMMAND_TEXT and CONNECTION to align w/ C# standards. There are also other instances of poor variable casing and naming throughout this function. ROWS_PER_PAGE is cased as a local constant, which should hopefully sound like an oxymoron. This variable is even modified later on. It should be named something like pageCap or pageLim. Up next, we have MaxRow, which isn't even used. If it were, rather than being PascalCase, it should be CamelCase to align w/ C# standards for variables. I'd say dataAdapter and dataTable should just be adapter and table, since we already know what they're doing in this context. Now, applying these changes, we get:

public List<DataForExcelExport> GetData()
{
    var list = new List<DataForExcelExport>();
    int pageLim = 41;

    var adapter = new Sqladapter(selectCommandText, connectionString);
    var table = new table();
    adapter.Fill(table);
    bool flgLastPage = false;

    for (int i = 0; i < table.Rows.Count; i++)
    {
        DataRow dr = table.Rows[i];
        var item = new DataForExcelExport();
        SetItemData(dr, item, true);

        if (i + pageLim < table.Rows.Count)
        {
            SetItemData(table.Rows[i + pageLim], item, false);
        }

        list.Add(item);

        if (flgLastPage == false)
        {
            if ((i + 1) % pageLim == 0)
            {
                i += pageLim;

                int remaining = table.Rows.Count - (i + 1);
                if (remaining < 2 * pageLim)
                {
                    pageLim = remaining / 2 + remaining % 2;
                    flgLastPage = true;
                }
            }
        }

        if ((i + pageLim) == table.Rows.Count)
        {
            break;
        }
    }

    return list;
}

The looping is pretty straightforward; you just want to get a certain number of data entries w/in a page limit. To that extent, you could just do something like this:

int pageLim = 80;

for (int i = 0; i < pageLim && i < table.Rows.Count; ++i)
{
    // set up `item` var using `row`
    list.Add(item);
}

This will create a single item page. Now, if you want multiple pages, I'd suggest looking into a Dictionary rather than using index-modulo magic in a flat list. This way, you can have page names rather than just page numbers, or any other object. If all you'll use are page numbers, a multi-dimensional list should suffice. That would look something like this:

int i = -1;
int pageLim = 80;
List pages = new List();
List page = new List();

foreach (var row in table.Rows)
{
    ++i;

    // set up `item` var using `row`

    if (i < pageLim) {
        page.Add(item);
    } else if (i == (pageLim - 1) || i == (table.Rows.Count - 1)) {
        page.Add(item); // add the last item
        pages.Add(page);
        page.clear();
        i = -1;
    }
}

Then just return pages, a list of lists w/ at most 80 items per page, and access a page like pages[0]. If pages have a lot of metadata, creating a Page object and just having a flat list of that is also possible.

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