Refactoring code into a simpler method

Question

I have some code that loops over a number and populates a list using other lists. I think I can refactor it somehow to make it look nicer but not sure the best way to do it.

Here is the code:

for (int i = 0; i < loopCount; i++)
{
 switch (NoOfRows)
 {
 case 1:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]); 
 break;
 case 2:
 if (InputList1.Count > i) 
 ExpectedValues.Add(InputList1[i]); 
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 break;
 case 3:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]);
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 if (InputList3.Count > i)
 ExpectedValues.Add(InputList3[i]);
 break;
 case 4:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]);
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 if (InputList3.Count > i)
 ExpectedValues.Add(InputList3[i]);
 if (InputList4.Count > i)
 ExpectedValues.Add(InputList4[i]);
 break;
 case 5:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]);
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 if (InputList3.Count > i)
 ExpectedValues.Add(InputList3[i]);
 if (InputList4.Count > i)
 ExpectedValues.Add(InputList4[i]);
 if (InputList5.Count > i)
 ExpectedValues.Add(InputList5[i]);
 break;
 case 6:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]);
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 if (InputList3.Count > i)
 ExpectedValues.Add(InputList3[i]);
 if (InputList4.Count > i)
 ExpectedValues.Add(InputList4[i]);
 if (InputList5.Count > i)
 ExpectedValues.Add(InputList5[i]);
 if (InputList6.Count > i)
 ExpectedValues.Add(InputList6[i]);
 break;
 case 7:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]);
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 if (InputList3.Count > i)
 ExpectedValues.Add(InputList3[i]);
 if (InputList4.Count > i)
 ExpectedValues.Add(InputList4[i]);
 if (InputList5.Count > i)
 ExpectedValues.Add(InputList5[i]);
 if (InputList6.Count > i)
 ExpectedValues.Add(InputList6[i]);
 if (InputList7.Count > i)
 ExpectedValues.Add(InputList7[i]);
 break;
 case 8:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]);
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 if (InputList3.Count > i)
 ExpectedValues.Add(InputList3[i]);
 if (InputList4.Count > i)
 ExpectedValues.Add(InputList4[i]);
 if (InputList5.Count > i)
 ExpectedValues.Add(InputList5[i]);
 if (InputList6.Count > i)
 ExpectedValues.Add(InputList6[i]);
 if (InputList7.Count > i)
 ExpectedValues.Add(InputList7[i]);
 if (InputList8.Count > i)
 ExpectedValues.Add(InputList8[i]);
 break;
 case 9:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]);
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 if (InputList3.Count > i)
 ExpectedValues.Add(InputList3[i]);
 if (InputList4.Count > i)
 ExpectedValues.Add(InputList4[i]);
 if (InputList5.Count > i)
 ExpectedValues.Add(InputList5[i]);
 if (InputList6.Count > i)
 ExpectedValues.Add(InputList6[i]);
 if (InputList7.Count > i)
 ExpectedValues.Add(InputList7[i]);
 if (InputList8.Count > i)
 ExpectedValues.Add(InputList8[i]);
 if (InputList9.Count > i)
 ExpectedValues.Add(InputList9[i]);
 break;
 case 10:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]);
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 if (InputList3.Count > i)
 ExpectedValues.Add(InputList3[i]);
 if (InputList4.Count > i)
 ExpectedValues.Add(InputList4[i]);
 if (InputList5.Count > i)
 ExpectedValues.Add(InputList5[i]);
 if (InputList6.Count > i)
 ExpectedValues.Add(InputList6[i]);
 if (InputList7.Count > i)
 ExpectedValues.Add(InputList7[i]);
 if (InputList8.Count > i)
 ExpectedValues.Add(InputList8[i]);
 if (InputList9.Count > i)
 ExpectedValues.Add(InputList9[i]);
 if (InputList10.Count > i)
 ExpectedValues.Add(InputList10[i]);
 break;
 }
}

• sinelaw: And how/where are InputList1..10 defined?
• MichaelT: You can put all InputLists into array of inputLists and use a simple for
• Jon: @Jamiec Not sure how to move it, copy & paste?
• Jon: @sinelaw they are class properties
• sinelaw: @Jon, ok but why do you define them like that? Why not make a list of lists?

Answer 1

The alternate method of Dr. Andrew Burnett-Thom would be to use a Dictionary<int, InputList>:

 var dic = new Dictionary<int, InputList>();
 // add InputLists
 dic.Add(0, InputList1);
 dic.Add(1, InputLIst2);
 //etc...
 for( int i = 0; i < loopCount; i++ )
 {
 for( int j = 0; j < NoOfRows; j++ )
 {
 if( dic[j].Count > i )
 {
 ExpectedValues.Add( dic[j][i] );
 }
 }
 }

• Khanzor: Except you need to index into dic[j] to get the value.
• Metro Smurf: @Khanzor I'm not following... what do you mean?
• Khanzor: The result of dic[j] will be an InputList, you want the ith value of that list, right?
• Metro Smurf: @Khanzor good catch!
• Metro Smurf: Not sure why the downvote... oh well.

Answer 2

Yes, a very simple way to do this is to have a number of methods and store delegates to them in a dictionary, keyed on NumberOfRows.

It'll look prettier but doesn't add any functional benefit. For instance:

 switch (NoOfRows)
 {
 case 1:
 if (InputList1.Count > i)
 ExpectedValues.Add(InputList1[i]); 
 break;
 case 2:
 if (InputList1.Count > i) 
 ExpectedValues.Add(InputList1[i]); 
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
 break;

Becomes

Dictionary<int, Action<int>> _operations;
public void Foo()
{
 // Create methods and store in a dictionary once
 if (_operations == null)
 {
 _operations = new Dictionary<int, Action>();
 _operations.Add(1, ProcessOneRow); 
 _operations.Add(2, ProcessTwoRows); 
 // ... 
 }
 for (int i = 0; i < loopCount; i++)
 {
 // Invoke the delegate to the correct method
 var operation = _operations[NoOfRows];
 operation(i);
 }
}
public void ProcessOneRow(int i)
{
 if (InputList1.Count > i) ExpectedValues.Add(InputList1[i]);
}
public void ProcessTwoRows(int i)
{ 
 ProcessOneRow(i);
 if (InputList2.Count > i)
 ExpectedValues.Add(InputList2[i]);
}

To be honest looking at the above you ought to move your delegate lookup outside of the loop if NoOfRows does not change inside the loop.

• Metro Smurf: I was about to post something similar; Instead of an Action as the value in the Dictionary, use the InputListx. Then based on the number of rows, perform the same action on each list for i to n. The patter in the OP is each list is doing the same thing.
• Dr. Andrew Burnett-Thompson: Aha yes, flip-side of the coin. I didn't notice the InputLists were just incrementing each time. I suppose the correct answer to this question is going to be "Do what is readable and simple for developers to understand". If it's too funky then it detracts from code readability and might not be that valuable!
• Jon: @MetroSmurf Post your answer!
• Metro Smurf: @Jon - posted and done.

Answer 3

What about some Linq?

var allInputLists = new List<List<T>>
{
 InputList1, InputList2, 
 InputList3, InputList4, 
 InputList5, InputList6, 
 InputList7, InputList8, 
 InputList9, InputList10
};
var expectedValues = from i in Enumerable.Range(0, loopCount)
 from list in allInputLists.Take(NoOfRows - 1)
 where list.Count > i
 select list[i];

Or, in fluent syntax:

var expectedValues = Enumerable.Range(0, loopCount)
 .SelectMany(i => allInputLists.Take(NoOfRows - 1)
 .Where(list => list.Count > i)
 .Select(list => list[i])
 );

Although the query syntax in this case is a bit nicer.

• Jon: can't quite see whats going on here, could you explain it more or use fluent linq?
• Khanzor: Enumerable.Range is like your for loop, and for each of the values that are generated, that walks through all the lists (so a nested for loop) up to the NoOfRows, checks that they have more elements than the current i, and "joins" together all those elements to build your ExpectedValues from scratch.