5
\$\begingroup\$

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:

Output

\$\endgroup\$
  • \$\begingroup\$ What's the point of this method if you are already telling him what to output ? Shouldn't you let the code decide ? \$\endgroup\$ – Denis Apr 21 '16 at 8:29
  • \$\begingroup\$ I'm not sure I understand what you mean. The code is deciding the output based on the input from the parameters. \$\endgroup\$ – Dumpen Apr 21 '16 at 8:40
  • \$\begingroup\$ The parameter itemsToAdd is what I mean. You basically have the outputed values in there. Am I wrong ? \$\endgroup\$ – Denis Apr 21 '16 at 8:53
  • \$\begingroup\$ The values are there yes, but what this code does is figure out where the output values should go in what order (It could be changed so its every 2th element instead) . I could just return items + itemsToAdd, but that would just add itemsToAdd at the end of the output. \$\endgroup\$ – Dumpen Apr 21 '16 at 8:56
  • \$\begingroup\$ Well figuring out the nth element is trivial. You can use just 1 for loop and if the the let's say i is the value in the for you can just check if it's multiple of our nth number. If so we output "item to add {0}",items[i] this requires you to create an int array and output "item {0}",array[i]. Wouldn't that work just fine ? If you want to still output the first indexes you can do an inner for loop that will start at array.Length - nth number + startingIndex starting index will incrase with 1 each time a sequence is printed. \$\endgroup\$ – Denis Apr 21 '16 at 8:59
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Did you test this? \$\endgroup\$ – venerik Apr 21 '16 at 20:50

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.