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;
    }

}

You can put all InputLists into array of inputLists and use a simple for — MichaelT, Commented Apr 26, 2012 at 13:20
@Jon, ok but why do you define them like that? Why not make a list of lists? — sinelaw, Commented Apr 26, 2012 at 13:22

Community · Accepted Answer · 2017-05-23 12:41:03Z

2

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] );
            }
        }
    }

edited May 23, 2017 at 12:41

CommunityBot

1

answered Apr 26, 2012 at 13:39

Metro Smurf

1234 bronze badges

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

Add a comment |

Dr. Andrew Burnett-Thompson · Accepted Answer · 2012-04-26 13:25:16Z

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.

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, Commented Apr 26, 2012 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, Commented Apr 26, 2012 at 13:30

Khanzor · Accepted Answer · 2012-04-26 13:27:12Z

1

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.

answered Apr 26, 2012 at 13:27

Khanzor

1735 bronze badges

1

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

Add a comment |

Stack Exchange Network

Refactoring code into a simpler method

3 Answers 3

Your Answer

Not the answer you're looking for? Browse other questions tagged
c#
.net
or ask your own question.

Hot Network Questions

Refactoring code into a simpler method

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c#.net or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
c#
.net
or ask your own question.