# Refactoring code into a simpler method

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)
break;
case 2:
if (InputList1.Count > i)
if (InputList2.Count > i)
break;
case 3:
if (InputList1.Count > i)
if (InputList2.Count > i)
if (InputList3.Count > i)
break;
case 4:
if (InputList1.Count > i)
if (InputList2.Count > i)
if (InputList3.Count > i)
if (InputList4.Count > i)
break;

case 5:
if (InputList1.Count > i)
if (InputList2.Count > i)
if (InputList3.Count > i)
if (InputList4.Count > i)
if (InputList5.Count > i)
break;
case 6:
if (InputList1.Count > i)
if (InputList2.Count > i)
if (InputList3.Count > i)
if (InputList4.Count > i)
if (InputList5.Count > i)
if (InputList6.Count > i)
break;

case 7:
if (InputList1.Count > i)
if (InputList2.Count > i)
if (InputList3.Count > i)
if (InputList4.Count > i)
if (InputList5.Count > i)
if (InputList6.Count > i)
if (InputList7.Count > i)
break;

case 8:
if (InputList1.Count > i)
if (InputList2.Count > i)
if (InputList3.Count > i)
if (InputList4.Count > i)
if (InputList5.Count > i)
if (InputList6.Count > i)
if (InputList7.Count > i)
if (InputList8.Count > i)
break;

case 9:
if (InputList1.Count > i)
if (InputList2.Count > i)
if (InputList3.Count > i)
if (InputList4.Count > i)
if (InputList5.Count > i)
if (InputList6.Count > i)
if (InputList7.Count > i)
if (InputList8.Count > i)
if (InputList9.Count > i)
break;

case 10:
if (InputList1.Count > i)
if (InputList2.Count > i)
if (InputList3.Count > i)
if (InputList4.Count > i)
if (InputList5.Count > i)
if (InputList6.Count > i)
if (InputList7.Count > i)
if (InputList8.Count > i)
if (InputList9.Count > i)
if (InputList10.Count > i)
break;
}

}

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

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

    var dic = new Dictionary<int, InputList>();
//etc...

for( int i = 0; i < loopCount; i++ )
{
for( int j = 0; j < NoOfRows; j++ )
{
if( dic[j].Count > i )
{
}
}
}

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

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)
break;
case 2:
if (InputList1.Count > i)
if (InputList2.Count > 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>();
// ...
}

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)
{
}

public void ProcessTwoRows(int i)
{
ProcessOneRow(i);
if (InputList2.Count > 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.

• 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. – Metro Smurf Apr 26 '12 at 13:28
• 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! – Dr. Andrew Burnett-Thompson Apr 26 '12 at 13:30
• @MetroSmurf Post your answer! – Jon Apr 26 '12 at 13:38
• @Jon - posted and done. – Metro Smurf Apr 26 '12 at 13:39

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.

• can't quite see whats going on here, could you explain it more or use fluent linq? – Jon Apr 26 '12 at 13:33
• 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. – Khanzor Apr 26 '12 at 13:36