24
\$\begingroup\$

I have the following code that loops through a collection of objects and dumps different properties out to Excel. The whole j++ on every other line doesn't seem very elegant. Is there a more elegant way to have this functionality where I loop through objects in a collection and dump out properties?

int rowIndex = 2;
foreach (BookInfo book in books)
{
    int j = 1;
    excelExport.SetCell(j, rowIndex, book.BookId);
    j++;
    excelExport.SetCell(j, rowIndex, book.Book);
    j++;
    excelExport.SetCell(j, rowIndex, book.System);
    j++;
    excelExport.SetCell(j, rowIndex, book.Age);
    j++;
    excelExport.SetCell(j, rowIndex, book.StartDate);
    j++;
    excelExport.SetCell(j, rowIndex, book.Pages);
    rowIndex++;
}

I am constantly adding new columns at the beginning and middle, so I want to avoid hard coding column names.

\$\endgroup\$

migrated from stackoverflow.com Dec 29 '11 at 16:40

This question came from our site for professional and enthusiast programmers.

  • 6
    \$\begingroup\$ You're basically hard-coding it. You could just as easily write each line in the format, "excelExport.SetCell(1, rowIndex, book.BookId)" etc. and accomplish the same thing with 1/2 the lines of code. \$\endgroup\$ – Moozhe Dec 28 '11 at 21:48
  • \$\begingroup\$ @Moozhe - but the code is more flexible for change and i am constantly adding new columns at the beginnign \$\endgroup\$ – leora Dec 28 '11 at 21:55
  • 5
    \$\begingroup\$ Won't excelExport.SetCell(j++, rowIndex, book.BookId); get you where you want to be? \$\endgroup\$ – user92546 Dec 28 '11 at 21:59
  • \$\begingroup\$ You say in other comments that you want to easily add columns, but that is a dangerous thing to do. You change the meaning of columns all the time. This is why it is better to use constants for the column index. What if you want to keep a column empty? I would certainly create a method that performs the SetCell operations on a single book as well. This method is bound to grow in time, so keep it neat. \$\endgroup\$ – owlstead Dec 29 '11 at 15:39
30
\$\begingroup\$

I think your code is perfectly understandable as it is.

Here's an idea though. (In case it is not clear: the suggestions here are more for amusement and edification than a serious suggestion. The original procedural code is just fine, but it is interesting to see how it might be done in a functional style.)

var funcs = new List<Func<BookInfo, object>>()
{
    info=>info.BookId,
    info=>info.Book,
    info=>info.System // etc.
}
int rowIndex = 2;
foreach (BookInfo bookInfo in books)
{
    int columnIndex = 1;
    foreach(var func in funcs)
    {
        excelExport.SetCell(columnIndex, rowIndex, func(bookInfo));
        columnIndex += 1;
    }
    rowIndex += 1;
}

However, this still has a disappointingly large number of variable mutations. Why do we need to have local variables to track the rows and columns at all? That's the inelegant part that you want to eliminate.

Can't tell if trolling...or just addicted to lambdas

Oh, we're just getting started here. I have barely yet even begun to use lambdas. How about this?

var funcs = new List<Func<BookInfo, object>>()
{
    info=>info.BookId,
    info=>info.Book,
    info=>info.System // etc.
}
var cells = bookInfos.SelectMany(
  (bookInfo, row)=>
    funcs.Select(
      (func, col)=>
        new {row, col, item = func(bookInfo)}));
foreach(var cell in cells)
    excel.SetCell(cell.col, cell.row, cell.item);

There, now we've got a selector that contains a lambda that contains a selector that contains a lambda that iterates over a list of lambdas. We've also gotten rid of every index mutation.

That of course has far too many explanatory local variables. We should be able to do this without mutating any variable except the loop variable. Let's eliminate all the variable mutations except one:

foreach(var cell in 
  bookInfos.SelectMany(
    (bookInfo, row)=>
      new List<Func<BookInfo, object>>()
      {
        info=>info.BookId,
        info=>info.Book,
        info=>info.System // etc.
      }.Select(
        (func, col)=>
          new {row, col, item = func(bookInfo)})))
  excel.SetCell(cell.col, cell.row, cell.item);

And now we've illustrated the old saying: every programming language eventually resembles Lisp -- badly.

As Scott Rippey points out in his answer, we actually don't need to be capturing the properties as lambdas at all; we could just capture the values:

foreach(var cell in 
  bookInfos.SelectMany(
    (bookInfo, row)=>
      new object[]
      {
        bookInfo.BookId,
        bookInfo.Book,
        bookInfo.System // etc.
      }.Select(
        (item, col)=>
          new {row, col, item})))
  excel.SetCell(cell.col, cell.row, cell.item);

Which is actually not too bad.

But like I said, your original code is just fine.

\$\endgroup\$
  • 8
    \$\begingroup\$ Can't tell if trolling...or just addicted to lambdas \$\endgroup\$ – cHao Dec 28 '11 at 22:00
  • 7
    \$\begingroup\$ @cHao: Oh we're just getting started here. \$\endgroup\$ – Eric Lippert Dec 28 '11 at 22:06
  • 2
    \$\begingroup\$ @EricLippert My answer is the same as yours ... except without the crazy addiction to lambdas. Crazy. \$\endgroup\$ – Scott Rippey Dec 28 '11 at 22:38
  • \$\begingroup\$ Wow I like the (bookInfo, row)=> new List<Func<BookInfo, object>>(). Nice. :-) \$\endgroup\$ – ΩmegaMan Dec 28 '11 at 22:44
  • 5
    \$\begingroup\$ @phoog: Hey, I'm being totally productive here. I just added a new error message to overload resolution and... that's about it. \$\endgroup\$ – Eric Lippert Dec 28 '11 at 22:57
25
\$\begingroup\$

This is super simple; use an array and a for-loop:

int rowIndex = 2;
foreach (BookInfo book in books)
{
    var columns = new object[]{
        book.BookId,
        book.Book,
        book.System,
        book.Age,
        book.StartDate,
        book.Pages,
    };
    for (int j = 0; j < columns.Length; j++) {
        excelExport.SetCell(j + 1, rowIndex, columns[j]);
    }
    rowIndex++;
}
\$\endgroup\$
8
\$\begingroup\$

This

excelExport.SetCell(j, rowIndex, book.BookId);
j++;

is equivalent to this:

excelExport.SetCell(j++, rowIndex, book.BookId);

<sarcasm> Now you have one line doing two things! </sarcasm>

Now admittedly this is not a great solution but it does address your concerns of appearance. Like Eric Lippert noted, there are reasons you shouldn't do this.

In your comments, you've noted that these values are evolving. With that in mind, consider the Open-Closed Principle where your export code should be open for extension but closed for modification. Since you are "constantly adding to the export," you're obviously violating the closed part of the principle.

A better solution might be to have the BookInfo define what it exports.

foreach(var book in books) 
{
    var columnIndex = 1;

    foreach(var exportValue in book.ExportValues)
    {
         excelExport.SetCell(columnIndex, rowIndex, exportValue);
         columnIndex += 1;
    }

    rowIndex++; 
}

With the code above, the Exporter is now closed for modification (no reason to change) but open for extension (book.ExportValues can grow/contract as needed).

\$\endgroup\$
  • \$\begingroup\$ is your point that your suggestion isn't recommended ? \$\endgroup\$ – leora Dec 28 '11 at 21:47
  • 4
    \$\begingroup\$ And one line doing two things is worse. One statement should do one thing; statements that do multiple things are harder to understand, harder to refactor, and harder to debug than statements that do one thing. \$\endgroup\$ – Eric Lippert Dec 28 '11 at 21:48
  • 1
    \$\begingroup\$ I would use the "excelExport.SetCell(j++, rowIndex, book.BookId)" solution. I obviosuly agree that you shouldn't do things at once just for the sake of it, but seriously, I think "applying a function to the next column" is simpler than "applying a function to the current column" and then "incrementing the column". It's a shame that there's no excellent syntax for this, but (a) I think using a list is probably too complicated and (b) hardcoding the numbers makes it harder to change the order of lines later. I still think "excelExport.SetCell(j++, rowIndex, book.BookId)" is simplest and best. \$\endgroup\$ – Jack V. Jan 12 '12 at 16:32
4
\$\begingroup\$

Every time you're confronted with code that doesn't feel quite right, try to imagine what you'd like the code to look like. Often, you can create an abstraction that better communicates the intent of the code, easing readability and future maintainability.

In this case, encapsulate the tedious code in a higher abstraction to have this result:

var row = new RowFiller(excelExport, startRowIndex: 2, startColumnIndex: 1);
foreach (BookInfo book in books) {
  row.Put(book.BookId);
  row.Put(book.Book);
  row.Put(book.System);
  row.Put(book.Age);
  row.Put(book.StartDate);
  row.Put(book.Pages);
  row.Skip();
}    

I thought it would be good to have an object that represents a row that I can put values on, that I can also skip to start putting values in the next row, always correctly taking care of rows and column indexes for me. The resulting code is easier to write, read and change.


This is the needed RowFiller class:

public class RowFiller {

  private readonly Excel excelExport;
  private readonly int startColumnIndex;
  private int currentRowIndex;
  private int currentColumnIndex;

  public RowFiller(Excel excelExport, int startRowIndex, int startColumnIndex) {
    this.excelExport = excelExport;
    this.currentRowIndex = startRowIndex;
    this.startColumnIndex = startColumnIndex;
    this.currentColumnIndex = startColumnIndex;
  }

  public void Put(object value) {
    excelExport.SetCell(currentColumnIndex, currentRowIndex, value);
    currentColumnIndex++;
  }

  public void Skip() {
    currentRowIndex++;
    currentColumnIndex = startColumnIndex;
  }

}

(Note: I don't know the type of excelExport, so I just assumed it was Excel)

\$\endgroup\$
2
\$\begingroup\$

Since you're redeclaring j on every iteration, its values are effectively static. So a "more elegant" way to do this would be:

foreach (BookInfo book in books)
{
    excelExport.SetCell(1, rowIndex, book.BookId);
    excelExport.SetCell(2, rowIndex, book.Book);
    excelExport.SetCell(3, rowIndex, book.System);
    excelExport.SetCell(4, rowIndex, book.Age);
    excelExport.SetCell(5, rowIndex, book.StartDate);
    excelExport.SetCell(6, rowIndex, book.Pages);

    rowIndex++;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ because i am constantly adding columns at the front and i don't want to have to shift the column number of every other column around every time i need to inject a new column in the middle\ \$\endgroup\$ – leora Dec 28 '11 at 21:51
  • 5
    \$\begingroup\$ @leora Then that additional requirement should go in the question. We can't read your mind - as far as readers of the question can tell, there's no reason to have a variable. \$\endgroup\$ – djacobson Dec 28 '11 at 21:53
2
\$\begingroup\$

I, personally, would avoid using j++ to populate Excel cells, and instead would use constants. Something like this:

foreach (BookInfo book in books)
{
    excelExport.SetCell(Constants.BookIdCellIndex, rowIndex, book.BookId);
    excelExport.SetCell(/*index of other cell*/ .....);
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I wouldn't. Too enterprisey. Makes for one more place to have to look to figure out what exactly the code's doing. This really shouldn't be that complicated. \$\endgroup\$ – cHao Dec 28 '11 at 21:57
  • \$\begingroup\$ @cHao: consider that one day you decide to reorganize the columns in Excel. How you gonna handle it inthis cycle code in "easy" way. ?? In case of index declaration, the only thing you need to change, is the index of the constant. The code is much robust and scallable then that one which uses a loop var. \$\endgroup\$ – Tigran Dec 28 '11 at 22:02
  • \$\begingroup\$ Except for that part about having to go halfway across the app to figure out what Constants.BookIdCellIndex is or what it's for. If you want it hard-coded, then hard-code it. If you want it flexible, then pass an array with stuff in the order you want. Not this. This looks like it was cooked up by a Java junkie. (BTW, not my downvote.) \$\endgroup\$ – cHao Dec 28 '11 at 22:06
  • 1
    \$\begingroup\$ @cHao: bah!... What if I have a function which reads the data from the same Excel. Like a good developer you will move the constants declaration out. just an example, to say that, the issue you pointing out is context dependent, so irrelevant for this question. Good luck. \$\endgroup\$ – Tigran Dec 28 '11 at 22:28
  • 2
    \$\begingroup\$ If you need those constants in two distinct places, then go for it. But til then, YAGNI. And if i'm going to be using the same constants for reading and writing, i don't want them easily modifiable, especially from halfway across the app. I want those suckers etched in stone, lest some schmuck decide to move some columns around and break backward compatibility. \$\endgroup\$ – cHao Dec 29 '11 at 6:05
0
\$\begingroup\$

I would advocate the column-index constant approach suggested by Bryan, but with a change. The change makes it easier for you to support the requirement of easy maintainability when inserting columns near the front:

    const int BOOK_ID    = 1; 
    const int BOOK       = BOOK_ID + 1; 
    const int SYSTEM     = BOOK + 1; 
    const int AGE        = SYSTEM + 1; 
    const int START_DATE = AGE + 1; 
    const int PAGES      = PAGES + 1; 

Adding a column between book and system? No problem:

    const int BOOK_ID    = 1;              //unchanged line
    const int BOOK       = BOOK_ID + 1;    //unchanged line
    const int NEW_COLUMN = BOOK + 1;       //new line
    const int SYSTEM     = NEW_COLUMN + 1; //changed line
    const int AGE        = SYSTEM + 1;     //unchanged line
    const int START_DATE = AGE + 1;        //unchanged line
    const int PAGES      = PAGES + 1;      //unchanged line
\$\endgroup\$
0
\$\begingroup\$

Since it appears as though your column definitions are static, I would create contants for your column indexes

const int BookIdColumn = 1;
const int BookColumn = 2;
const int SystemColumn = 3;
const int AgeColumn = 4;
const int StartDateColumn = 5;
const int PagesColumn = 6;

int rowIndex = 2;
foreach (BookInfo book in books)
{
      excelExport.SetCell(BookIdColumn, rowIndex, book.BookId);
      excelExport.SetCell(BookColumn, rowIndex, book.Book);
      excelExport.SetCell(SystemColumn, rowIndex, book.System);
      excelExport.SetCell(AgeColumn, rowIndex, book.Age);
      excelExport.SetCell(StartDateColumn, rowIndex, book.StartDate);
      excelExport.SetCell(PagesColumn, rowIndex, book.Pages);
}
\$\endgroup\$
0
\$\begingroup\$

One can use reflection to avoid having to hard code the properties of the book. I take the direction that all the public properties need to be shown. Then I use the index of the book to define the row and the index of the property to define the j in your example.

Note: I commented out the actual call to excel which would be your actual call; but left it out so it can run.

This teaches Linq, which I feel separates the mid level C# developers from the senior level because they now think in terms of IEnumerable/IQuerable on all data structures.

The below will run in LinqPad:

void Main()
{
    var books = new List<BookInfo>() { new BookInfo() { BookId = 1, Book="War N Peace", StartDate= DateTime.Now  },
                                       new BookInfo() { BookId = 2, Book="Visual Basic .NET Code Security Handbook", StartDate=DateTime.Now.AddYears(1) }};


    var publicProps = typeof(BookInfo).GetProperties( BindingFlags.Instance | BindingFlags.Public );

    books.Select ((bk, index) => new { Book = bk, BookIndex = index} )
         .ToList()
         .ForEach(bk => publicProps.Select ((prp, index) => new {Prop = prp, Index = index } )
                                   .ToList()
                                   .ForEach(pp => //excelExport.SetCell(bk.BookIndex,  pp.Index, pp.Prop.GetValue(bk.Book, null)); 
                                                  Console.WriteLine ("RowIndex as ({0}) J as ({1}) reflected value as ({2})", bk.BookIndex,  pp.Index, pp.Prop.GetValue(bk.Book, null)))
                  );

/* Output
RowIndex as (0) J as (0) reflected value as (1)
RowIndex as (0) J as (1) reflected value as (War N Peace)
RowIndex as (0) J as (2) reflected value as (12/28/2011 3:33:15 PM)
RowIndex as (1) J as (0) reflected value as (2)
RowIndex as (1) J as (1) reflected value as (Visual Basic .NET Code Security Handbook)
RowIndex as (1) J as (2) reflected value as (12/28/2012 3:33:15 PM)
*/                                 


}

// Define other methods and classes here

public class BookInfo
{
    public int BookId { get; set; }
    public string Book { get; set; }
    public DateTime StartDate { get; set; }

}

EDIT: Removed redundant tolist.

\$\endgroup\$
  • \$\begingroup\$ @jamal I am honored that you would consider my answer enough to edit it to its proper verbiage. The only thing which was in doubt was the change of tack to task. My original intent was tack as to "tack into the wind", as a direction or really course. 'Task' as your change would have worked per-se, but I knew the original meaning and changed it to direction for a better understanding. Thanks. \$\endgroup\$ – ΩmegaMan Jun 9 '14 at 4:14
  • \$\begingroup\$ Thanks for the clarification. I changed it as it seemed like a typo, and I just wanted to be sure. \$\endgroup\$ – Jamal Jun 9 '14 at 4:19

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.