Building equal number of records list object

Question

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
_________

Answer 1

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.

Answer 2

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.