Return a multiple-enumeration-safe IEnumerable when a value needs resetting each time

Question

The following code fails multiple enumeration because the existingNames hash set still contains the results of the last enumeration, thus the numeric suffixes are advanced more than is correct. What's an elegant way to soup up this method so that it works correctly upon multiple enumeration?

public static IEnumerable<TOutput> UniquifyNames<TSource, TOutput>(
   this IEnumerable<TSource> source,
   Func<TSource, string> nameSelector,
   Func<TSource, string, TOutput> resultProjection
) {
   HashSet<string> existingNames = new HashSet<string>();
   return source
      .Select(item => {
         string name = nameSelector(item);
         return resultProjection(
            item,
            Enumerable.Range(1, int.MaxValue)
               .Select(i => {
                  string suffix = i == 1
                     ? ""
                     : (name.EndsWithDigit() ? "-" : "") + i.ToString();
                  return $@"{name}{suffix}";
               })
               .First(candidateName => existingNames.Add(candidateName))
         );
      });
}

private static bool EndsWithDigit(this string value)
   => !string.IsNullOrEmpty(value) && "0123456789".Contains(value[value.Length - 1]);

I thought about creating an extension method such as UponEnumeration to wrap the outer enumerable, which would take a callback Action to run when enumeration began again (and which could be used to reset the HashSet). Is that a good idea?

I just realized it's not a good idea as stated, because the same resulting IEnumerable could be enumerated by different classes at the same time (begin enumerating in one place, while the other was still partway through enumeration, so things would break after resuming enumeration, because the HashSet got cleared). It sounds like the best thing to do is simply ToList() but I really would like to preserve lazy evaluation if possible.

Answer 1

By making your code a deferred IEnumerable itself when other people run it multiple times it will also be run multiple times.

public static IEnumerable<TOutput> UniquifyNames<TSource, TOutput>(
   this IEnumerable<TSource> source,
   Func<TSource, string> nameSelector,
   Func<TSource, string, TOutput> resultProjection
) {
   HashSet<string> existingNames = new HashSet<string>();
   var items = source
      .Select(item => {
         string name = nameSelector(item);
         return resultProjection(
            item,
            Enumerable.Range(1, int.MaxValue)
               .Select(i => {
                  string suffix = i == 1
                     ? ""
                     : (name.EndsWithDigit() ? "-" : "") + i.ToString();
                  return $@"{name}{suffix}";
               })
               .First(candidateName => existingNames.Add(candidateName))
         );
      });
    foreach(TOutput item in items)
    {
        yield return item;
    }
}

Me personally, if I was doing this for real, I would "unroll" the LINQ queries and do their equivalents myself inside the foreach loop. Here is my first quick stab at changing it over.

public static IEnumerable<TOutput> UniquifyNames<TSource, TOutput>(
    this IEnumerable<TSource> source,
    Func<TSource, string> nameSelector,
    Func<TSource, string, TOutput> resultProjection
    )
{
    HashSet<string> existingNames = new HashSet<string>();
    foreach (TSource item in source)
    {
        string name = nameSelector(item);
        yield return resultProjection(item, GenerateName(name, existingNames));
    }
}

private static string GenerateName(string name, HashSet<string> existingNames)
{
    return Enumerable.Range(1, int.MaxValue)
        .Select(i =>
        {
            string suffix = i == 1
                ? ""
                : (name.EndsWithDigit() ? "-" : "") + i.ToString();
            return $@"{name}{suffix}";
        }).First(existingNames.Add);
}

Note that best practice for yielding/deferred IEnumerables is to check for null parameters in one method, and then return the result of the actual private implementation. This is so that the IEnumerable in the error case will throw immediately upon invocation/creation, instead of later once it is enumerated (possibly in code far away from the code that created it).

public static IEnumerable<TOutput> UniquifyNames<TSource, TOutput>(
   this IEnumerable<TSource> source,
   Func<TSource, string> nameSelector,
   Func<TSource, string, TOutput> resultProjection
) {
   if (source == null) {
      throw new ArgumentNullException(nameof(source));
   }
   if (nameSelector == null) {
      throw new ArgumentNullException(nameof(nameSelector));
   }
   if (resultProjection == null) {
      throw new ArgumentNullException(nameof(resultProjection));
   }
   return UniquifyNamesImpl(source, nameSelector, resultProjection);
}

Answer 2

I did come up with a way that works, but I don't know if it's good:

public class ResettingEnumerable<T> : IEnumerable<T> {
    private readonly Func<IEnumerable<T>> _enumerableFetcher;

    public ResettingEnumerable(Func<IEnumerable<T>> enumerableFetcher) {
        _enumerableFetcher = enumerableFetcher;
    }

    public IEnumerator<T> GetEnumerator() => _enumerableFetcher().GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

The body of UniquifyNames then turns into this:

return new ResettingEnumerable<TOutput>(() => {
   /* old body here */
};

After seeing Scott Chamberlain's answer, though, I think his idea is probably better: just write it as a yielding IEnumerable which has the property of being run anew each time GetEnumerator is called. This is a great general solution to the issue, where when multiple enumeration can't be tolerated, switch to a deferred IEnumerable.

---

For the record, I selected a slightly different final implementation. Initially, I wanted to preserve the lazy-evaluation aspect of an IEnumerable, where the set could be less than fully-enumerated and produce useful results. However, I realized that my goal of mutating any existing names as little as possible led me to choose a different algorithm that requires fully enumerating the list (to take all names as-is that can be taken, before beginning any numeric incrementing). Here is that solution for you:

private class NamedItem<TSource> {
   public TSource Item { get; set; }
   public string Name { get; set; }
}

private static bool EndsWithADigit(this string value) =>
   !string.IsNullOrEmpty(value) && "0123456789".Contains(value[value.Length - 1]);

private static string GetNumberedName(string name, int index) =>
   name + (index == 1 ? "" : name.EndsWithADigit() ? $"-{index}" : $"{index}");

private static bool ConditionalSetName<T>(
   NamedItem<T> namedItem, string name, HashSet<string> hashset
) {
   bool isNew = hashset.Add(name);
   if (isNew) { namedItem.Name = name; }
   return !isNew;
}

public static IEnumerable<TOutput> UniquifyNames<TSource, TOutput>(
   this IEnumerable<TSource> source,
   Func<TSource, string> nameSelector,
   Func<TSource, string, TOutput> resultProjection
) {
   var seen = new HashSet<string>();
   var result = source.Select((item, seq) => new NamedItem<TSource>{
      Item = item, Name = nameSelector(item)
   }).ToList();
   var remaining = result;
   int i = 1;
   do {
      remaining = remaining.Where(namedItem =>
         ConditionalSetName(namedItem, GetNumberedName(namedItem.Name, i++), seen)
      ).ToList();
   } while (remaining.Any());
   return result.Select(namedItem => resultProjection(namedItem.Item, namedItem.Name));
}

With this input:

"String2", "String", "String", "String3", "String3"

This gives the result:

"String2", "String", "String4", "String3", "String3-2"

This is better because the name String3 is left untouched.

My original implementation gave this result:

"String2", "String", "String3", "String3-2", "String3-3"

This is worse because it mutates the first String3 unnecessarily.