Add item after every nth step

Question

The purpose of the method is to add a list of items after every nth step (5 for example) where the input items are repeated if needed. I'm looking for an overall review and possible how to remove the use of the while and replace it with a for.

public static List<string> AddEveryNthStep(List<string> items, List<string> itemsToAdd, int nthStep)
{
    List<string> output = new List<string>();

    int itemIndex = 0;
    int itemsToAddIndex = 0;
    int index = 1;

    while (itemsToAddIndex < itemsToAdd.Count)
    {
        output.Add(items[itemIndex++]);

        if (itemIndex == items.Count)
        {
            itemIndex = 0;
        }

        // We are at at point where we want to add a item
        if (index % nthStep == 0)
        {
            output.Add(itemsToAdd[itemsToAddIndex++]);
        }

        index++;
    }

    return output;
}

Usage:

List<string> items = new List<string> { "Item 1", "Item 2", "Item 3", "Item 4" };
List<string> itemsToAdd = new List<string> { "Item to add 1", "Item to add 2", "Item to add 3" };

Console.WriteLine(String.Join(Environment.NewLine, AddEveryNthStep(items, itemsToAdd, 5)));

Output:

Answer 1

I think your code looks rather nice. Your use of a while loop seems good to me because the loop isn't a trivial one. See my explanation on this further down.

The name of your method is a bit odd. I can't think of a proper name though. I named my method Merge but that doesn't reveal the method's intent either.

You should add some safeguards on the input to prevent your code from throwing all kinds of unexpected exceptions.

You should add logic that handles empty lists.

Here's my take on the problem:

public List<String> Merge(List<String> sourceItems, List<String> insertItems, int step)
{
    if(sourceItems == null) throw new ArgumentNullException("sourceItems");
    if(insertItems == null) throw new ArgumentNullException("insertItems");
    if(step <= 0) throw new ArgumentException("Step must be greater than zero.");

    if(sourceItems.Any() == false) return insertItems;
    if(insertItems.Any() == false) return sourceItems;

    var result = new List<String>();

    var insertIndex = 0;
    var sourceIndex = 0;

    do {
        result.Add(sourceItems[sourceIndex++ % sourceItems.Count]);

        if(sourceIndex % step == 0)
        {
            result.Add(insertItems[insertIndex++]);
        }
    } while (insertIndex < insertItems.Count);

    return result;
}

do while vs for

So, why do I use a do while loop instead of a for loop? That's because in this case the while loop is easier to understand than a for loop. See what happens when I rewrite the loop to a for loop:

var sourceIndex = 0;

for(var insertIndex = 0; insertIndex < insertItems.Count;) {
    result.Add(sourceItems[sourceIndex++ % sourceItems.Count]);

    if(sourceIndex % step == 0)
    {
        result.Add(insertItems[insertIndex++]);
    }
}

The tricky part of this code is the missing iterator. Normally a for loop consists of an initializer, a condition and an iterator:

for(initializer; condition; iterator)
    body

In most cases the iterator gets incremented as long as the condition is not met. When the iterator is missing, programmers get confused: why is there no iterator! And only after examining the rest of the loop they discover the increase of insertIndex.

Answer 2

Loops and Indexes

Why are you using a while loop? A for loop keeps track of indexes for you. Admittedly you do still need to track one index yourself, but you can eliminate one to loop processing:

int addIndex = 0;
for (int i = 0; i < items.Count; i++) {
    output.Add(items[i]);

    if (i % nthStep == 0) {
        output.Add(itemsToAdd[addIndex++]);
    }

    if (i == (items.Count - 1) && addIndex < (itemsToAdd.Count - 1)) {
        i = 0;
    }
}

In words:

• define an index for the items you're adding
• loop through the items list, on every pass:
  • add the current item to the output list
  • if the current step is the nth step, add the next item to add and increase the index
  • if we've run out of items but have more left to add, reset the items index so we loop back through

Now you're only tracking and handling one index yourself, instead of the three that your while loop requires.