Add dash between more than two consecutive numbers and comma between other numbers in array

Question

I have an ordered array of integer values: 10, 11, 12, 13, 15, 16, 19 and I need to write a dash between first and last consecutive values when there are more than two consecutive values (10-13). Between other values I need to write a comma and whitespace. Output for this input must be 10-13, 15, 16, 19. I wrote a method that implements this:

class Program
{
    private static string AddDashesBetweenConsecutiveNumbers(int[] orderedArray)
    {
        if (orderedArray.Length == 0 || orderedArray == null)
        {
            return string.Empty;
        }
        var groupedArray = orderedArray.Select((x, i) => new { Difference = i - x, Value = x })
                .GroupBy(x => x.Difference)
                .Select(group => group.Select(a => a.Value))
                .Select(arr => arr.Count() > 2
                            ? $"{arr.First()}-{arr.Last()}"
                            : string.Join(", ", arr));
        return string.Join(", ", groupedArray);
    }
    static void Main(string[] args)
    {
        int[] array = { 10, 11, 12, 13, 15, 16, 19 };

        Console.WriteLine(AddDashesBetweenConsecutiveNumbers(array)); // Output: "10-13, 15, 16, 19"
    }
}

What I wrote seems to work, but this code has some disadvantages, especially in readability of the LINQ expression. So if you have any ideas how to improve this method or have better or alternative solution, that would be great to find out.

Answer 1

Sorry but no comments (yet) on the LINQ.

Your first conditional has a logic error. You should be checking if orderedArray is null before you check if its length is 0.

The method has a leap of faith that someone is inputting an array sorted in ascending order. There is nothing in your code to prevent otherwise.

What if someone wanted to pass in a List or IEnumerable? Restricting input to an array is fairly limiting.

The variable groupedArray is not an array, so the name is misleading. A quick reworking of all of the above is:

private static string AddDashesBetweenConsecutiveNumbers(IEnumerable<int> values)
{
    // Avoid Count() since it requires full list enumeration.
    // FirstOrDefault() would return default(int) or 0, which is not null.
    // Any() will find the first one without a full list enumeration.
    if (values == null || !values.Any())
    {
        return string.Empty;
    }

    // If the input values is already sorted, this will be fast.
    // If the input values is not already sorted, good thing we do it here.
    var orderedValues = values.OrderBy(x => x);

    var groupedValues = orderedValues.Select((x, i) => new { Difference = i - x, Value = x })
            .GroupBy(x => x.Difference)
            .Select(group => group.Select(a => a.Value))
            .Select(arr => arr.Count() > 2
                        ? $"{arr.First()}-{arr.Last()}"
                        : string.Join(", ", arr));

    return string.Join(", ", groupedValues);
}

I'm sure you are capable of writing a non-LINQ version if you desired. I include a version below.

private static string AddDashesBetweenConsecutiveNumbers2(IEnumerable<int> values)
{
// Avoid Count() since it requires full list enumeration.
// FirstOrDefault() would return default(int) or 0, which is not null.
if (values == null || !values.Any())
{
    return string.Empty;
}

// If the input values is already sorted, this will be fast.
// If the input values is not already sorted, good thing we do it here.
var orderedValues = values.OrderBy(x => x);

return string.Join(", ", GetGroupedIntegers(orderedValues));
}

private static IEnumerable<string> GetGroupedIntegers(IOrderedEnumerable<int> orderedValues)
{
var sublist = new List<int>();

Func<string> getSubListOutput = () =>
{
    switch (sublist.Count)
    {
        case 1:
            return sublist.First().ToString();
        case 2:
            return sublist.First().ToString() + ", " + sublist.Last().ToString();
        default:
            return sublist.First().ToString() + "-" + sublist.Last().ToString();
    }
};

foreach (var value in orderedValues)
{
    if (sublist.Count == 0 || sublist.Last() == value - 1)
    {
        sublist.Add(value);
    }
    else
    {
        yield return getSubListOutput();
        sublist = new List<int>() { value };
    }
}

yield return getSubListOutput();
}

Got to run to a meeting, but another flaw in both your original and my answer is that there is no check for distinct integers, but that can easily be done.

UPDATE: Corrected code as per comments.

Answer 2

if (orderedArray.Length == 0 || orderedArray == null)
{
    return string.Empty;
}

You are checking this in the wrong order. The null check should be first. The other condition is actually not necessasry at all because if the collection is empty the resulting string will be empty too.

---

As far as the rest of the code is concerned you can simplify it a little bit.

var groupedArray = orderedArray.Select((x, i) => new { Difference = i - x, Value = x })

This new can be replaced by an anonymous tuple that is cheaper than an object.

        .GroupBy(x => x.Difference)
        .Select(group => group.Select(a => a.Value))

This Select is not necessary. >ou can put it inside the GroupBy method as an element selector, here g => g.Value.

        .Select(arr => arr.Count() > 2
                    ? $"{arr.First()}-{arr.Last()}"
                    : string.Join(", ", arr));

I think it'd be a better idea to encapsulate this by creating a new Range type where you overload the ToString method:

readonly struct Range
{
    public Range(int min, int max)
    {
        Min = min;
        Max = max;
    }

    public int Min { get; }

    public int Max { get; }

    public static Range Create(int value) => new Range(value, value);

    public override string ToString() => Min == Max ? Min.ToString() : $"{Min}-{Max}";
}

The new version would then look like this:

var ranges =
    source
        .Select((x, i) => (Difference: i - x, Value: x))
        .GroupBy(x => x.Difference, g => g.Value)
        .SelectMany(
            g => g.Count() > 2
                ? new[] { new Range(g.First(), g.Last()) }
                : g.Select(Range.Create)
        );

return string.Join(", ", ranges);

Notice the new SelectMany that will give a collection of ranges so that you don't have to use string.Join multiple times. This makes it easier to replace the item delimiter because it now occurs only once.

Answer 3

As a followup to my comment on t3chb0ts answer, you can avoid the Range-struct in a way like this:

static string GroupNumbers(IEnumerable<int> source, string singleSeparator = ", ", string groupSeparator = "-")
{
  if (source == null)
    throw new ArgumentNullException(nameof(source));

  string toString(int min, int max) => min == max ? min.ToString() : (max == min + 1 ? $"{min}{singleSeparator}{max}" : $"{min}{groupSeparator}{max}");

  var ranges =
    source.OrderBy(x => x)
    .Select((x, i) => (Difference: i - x, Value: x))
    .GroupBy(x => x.Difference, g => g.Value)
    .Select(gr => toString(gr.First(), gr.Last()));

  return string.Join(singleSeparator, ranges);
}

Answer 4

Review

Don't trust parameters of public methods purely on their name orderedArray. I will address how to handle this further in this answer.

private static string AddDashesBetweenConsecutiveNumbers(int[] orderedArray) {

---

You mentioned

but this code has some disadvantages, especially in readability of the LINQ expression

We could create a generalized Linq extension to allow for processing adjacent values in a sequence. This greatly improves readability of the calling code.

---

Proposed Solution

My linq extension (see answers) could be used for this kind of problem. It is an analytical function that orders the source sequence, runs over it with look-behind to the previous item, and accepts a predicate for joining adjacent items in a result set as part of the outer multiset.

public static class LinqExtension
{
    public static IEnumerable<IEnumerable<TSource>> JoinBy<TSource, TOrderKey, TKey>(
        this IEnumerable<TSource> source,
        Func<TSource, TOrderKey> orderBy,
        Func<TSource, TKey> keySelector,
        Func<TKey, TKey, bool> join) {
        var results = new List<List<TSource>>();
        var orderedSource = new List<TSource>(source).OrderBy(orderBy).ToArray();

        if (orderedSource.Length > 0) {
            var group = new List<TSource> { orderedSource[0] };
            results.Add(group);
            if (orderedSource.Length > 1) {
                for (int i = 1; i < orderedSource.Length; i++) {
                    var lag = orderedSource[i - 1];
                    var current = orderedSource[i];
                    if (join(keySelector(lag), keySelector(current))) {
                        group.Add(current);
                    }
                    else {
                        group = new List<TSource> { current };
                        results.Add(group);
                    }
                }
            }
        }

        return results;
    }
}

Rendering could also be forwarded to a seperate method for readability and seperation of concerns.

private static string Render(IEnumerable<IEnumerable<int>> adjacentValuesMultiSet) 
{
    if (adjacentValuesMultiSet == null) return string.Empty;
    return string.Join(", ", adjacentValuesMultiSet.Select(group
        => group.Count() == 1 
            ? group.Single().ToString() 
            : string.Format("{0}-{1}", group.First(), group.Last())));
}

Your method can be rewritten as

private static string AddDashesBetweenConsecutiveNumbers(IEnumerable<int> values)
{
    if (values == null) return string.Empty;
    var adjacentValuesMultiSet = values.JoinBy(
        x => x, // <- the order by clause
        x => x,  // <- the target property to check
        (previous, current) => previous == current - 1); // <- the predicate
    return Render(adjacentValuesMultiSet);
}

Test case

public static void Main()
{
    int[] array = { 11, 10, 12, 13, 15, 16, 19 }; // <-- unordered
    Console.WriteLine(AddDashesBetweenConsecutiveNumbers(array));
}