Find consecutive numbers and turn them into ranges

Question

I have array

int[] arr = { -2, 0, 1, 2, 3, 4, 5, 8, 9, 11, 13, 15, 18, 22, 25, 28, 29, 30 };

I need to write function that shows it as string and if numbers are near, for example 1,2,3,4,5 - I need to show them 1-5.

I wrote this function in repository

public string[] GetArray()
{
    int[] arr = { -2, 0, 1, 2, 3, 4, 5, 8, 9, 11, 13, 15, 18, 22, 25, 28, 29, 30 };

    int? lastValue = arr.FirstOrDefault();

    var groupDataIntoAdjacentBlocks = arr.Segment(i =>
    {
        var result = lastValue != null && (lastValue != (i - 1));
        lastValue = i;
        return result;
    }
    );

    var convertSetsIntoRanges = groupDataIntoAdjacentBlocks.Select(z =>
    {
        var data = z.ToList();
        if (data.Count() == 1)
            return data.First().ToString();

        return data.First().ToString() + "-" + data.Last().ToString();
    });

    var finalresult = (string.Join(",", convertSetsIntoRanges));
    return new[] { finalresult };

}

And call it like this in controller

public JsonResult GetArray()
{
    var arrayresult = _arrayrepo.GetArray();
    return Json(arrayresult, JsonRequestBehavior.AllowGet);
}

Any suggestions how I can improve my code?

Answer 1

Letting a single method do everything is not such a good idea. You should rather separate the logic for finding consecutive numbers from creating a string from ranges and let them produce the final result by chaining them as extensions.

---

The first extension only knows how to find ranges. If you don't have range class/struct yet then you can use tuples.

public static IEnumerable<(int Min, int Max)> ToRanges(this IEnumerable<int> source)
{
    using (var e = source.GetEnumerator())
    {
        if (!e.MoveNext())
        {
            yield break;
        }

        var previous = e.Current;

        var range = (Min: previous, Max: e.Current);

        while (e.MoveNext())
        {
            var isConsecutive = e.Current - previous == 1;
            if (isConsecutive)
            {
                range = (range.Min, e.Current);
            }
            else
            {
                yield return range;
                range = (e.Current, e.Current);
            }
            previous = e.Current;
        }

        yield return range;
    }
}

The other extension consumes the result of ToRanges and returns a string.

public static string Stringify(this IEnumerable<(int Min, int Max)> ranges)
{
    var values = ranges.Select(range => $"{(IsEmpty(range) ? $"{range.Min}" : $"{range.Min}-{range.Max}")}");
    return string.Join(", ", values);

    bool IsEmpty((int Min, int Max) range)
    {
        return range.Max - range.Min == 0;
    }
}

---

The advantages of having two separated methods are: - you can focus on one task at a time - you are now able to test only one feature at a time - you can reuse ToRange to calculate their lengths if necessary

---

Other things that you should address are: - variable names should not sound like they were methods, e.g. groupDataIntoAdjacentBlocks; a much better name would be adjacentBlocks or something along the same lines - int? lastValue = arr.FirstOrDefault(); this is wrong since the default value for int is 0 and not null. - the result of the GetArray() method does not make much sense; why would you return an array of strings if there is always only one item? I guess this is not your real code but the range finding part and you edited it for the sake of the question so I won't comment on that one.

Answer 2

This was just going to be a quick answer... but I seem to have written a big wall of text: sorry about that.

GetArray API

I'm wondering whether there is a good reason that this is an instance method; rather, I'm assuming this is a very specific implementation of the GetArray method on some interface. I say this, because without that context this looks like a terrible API! You have a bizarrely named instanced method (what does GetArray mean?) which isn't using any instance state, and which has a very unusual return type: all of this makes sense, however, if this is an implementation of an interface which is specifically designed for producing JSON, but I'd still reckon you can come up with a better name!

Even if this is a very specific interface implementation, I'd be very much inclined to break most of this out into a (static) helper function, because currently you have all of the complexity of performing the task stuffed into a method which really only cares about the result, not how you got there, and which has some weird return value which is nothing to do with the grouping logic. I would be strongly inclined to break out 2 methods, one to perform the grouping and another to produce the strings from the groups, just because one feels like "data processing" and the other feels like "data formatting" and those are different ideas working with different types (just as producing the string and stuffing it into an array for the benefit of JSON are different ideas). You can go with whatever granularity you like, but I'd certainly want to pull the code for finalResult out:

/// <summary>
/// Produces a string representation of a sequence of integers, combining contiguous values into a single range
/// For example: {1, 2, 3, 5} -> "1-3,5"
/// </summary>
public static string ProduceContiguousIntegersString(IEnumerable<int> integers)
{
    if (integers == null)
        throw new ArgumentException("The list of integers may not be null", nameof(integers));

    // snip
}

Note the parameter type: you are only performing forward passes over the array, so there is no reason to limit the consumer to passing you an array: IEnumerable<T> is the simplest type you can get away with here as input. As always, some inline documentation (\\\) is always appreciated on any public API. I threw a null check in there as well just as an example: public APIs should always check input and throw meaningful exceptions; if this is just being used internally then it is less important, but still useful.

This is how you might call this new method:

public string[] GetArray()
{
    int[] arr = { -2, 0, 1, 2, 3, 4, 5, 8, 9, 11, 13, 15, 18, 22, 25, 28, 29, 30 };

    var finalResult = ProduceContiguousIntegersString(arr);
    return new[] { finalResult };
}

As RadarBob said, with arr baked in, this method just looks wrong. I've taken the liberty of changing "finalresult" to "finalResult", because it's two words, but I'm sure you could come up with a more meaningul name if you gave it some more thought.

lastValue

lastValue is weird. You seem to have made it nullable so that you can cope with empty arrays (which screams "arr isn't meant to be hard-coded"), but this is all a bit cryptic, and it isn't clear what the output will be if you do pass in an empty array. I would be inclined to provide an explicit "is the array empty" check.

A 'better' use of the nullability would to make it explicit that it isn't a meaningful value yet: indeed, you can replace arr.FirstOrDefault(); with null and your code will still work, and will be less confusing even! In fact... you can remove the check altogether, because that code will never run if the array is empty! Any such checks (e.g. that it isn't null) are overly defensive, and will only work to obscure bugs in future.

The 'real problem' with this code, is that Segment has a slightly odd API. I this Segment is a great idea, but it's kind-of-horrendous expecting the segmentation function to keep track of its own state, and it's not clear whether it calls this lambda for the first element or not. Assuming we can't mess with Segment, my instinct is to go with something like this:

public static string ProduceContiguousIntegersString(IEnumerable<int> integers)
{
    // handle special case of empty enumerable
    if (!integers.Any())
    {
        return "";
    }

    int previousValue = integers.First();
    var groupDataIntoAdjacentBlocks = integers.Segment(i =>
    {
        bool newSegment = previousValue != (i - 1);
        previousValue = i;
        return newSegment;
    }
    );

    // ...

To be really picky, I'd prefer it was called "previousValue", because "lastValue" might imply the last value in the array.

z.ToList()

What is z? I can forgive i in the first loop, but what is z? And why are you turning it into a List<T>? At worst you should be turning it into an array, since you don't need a dynamic data type, and rather than assigning to var, you could assign to IReadOnlyList<int>, which ensures you can't accidently change anything. Anyway, you are only performing simple operations on z (which I've renamed to block below), so there isn't much harm in leaving it as IEnumerable<T> or whatever it already is. (If you are concerned about the cost of Count(), then go with the T[] as IReadOnlyList approach).

    // ...

    var convertSetsIntoRanges = groupDataIntoAdjacentBlocks.Select(block =>
    {
        if (block.Count() == 1)
            return segment.First().ToString();

        return block.First().ToString() + "-" + block.Last().ToString();
    });

    var finalResult = (string.Join(",", convertSetsIntoRanges));
    return finalResult;
}

I'd also reconsider the variable name groupDataIntoAdjacentBlocks if I were you: to me it reads as though it produces some blocks which are adjacent, but really you are producing some blocks which contain adjacent elements.

Sorting??

As RadarBob suggests, it looks as though this code is expecting a sorted array. Perhaps you ought to sort the array before processing it? If you are worried about performance, you can do a first pass to check if the array is already sorted, and only sort it if necessary; otherwise, just document the parameters clearly.

Answer 3

I would probably make the following changes:

1. Rename the method to something more descriptive, like GetRangeString.
2. Make the method static. Since no other state is needed, it makes it easier on the client.
3. Have the method return a string, since that's really all it's creating.
4. Have the method take an integer array as input. As it stands now, since your method creates an array internally, it may as well just return a hard-coded string.
5. Build the range string on a single pass through the array. The code may not look as nice, but I believe it's much more efficient than creating segments, selecting those segments into lists, and building the ranges from those
6. Access elements by index (like arr[0]) rather than using extension methods like arr.First() if possible. If you look at the source code for IEnumerable.First(), you'll notice that it does a null check, does an IList cast, another null check, a count check, and finally returns the item at index[0]. It has to be faster just to do that in the first place.

I don't know what the Segment method does. Is that an extension method that you created? I would like to do a perf test between the your code and the code below, but cannot use your existing code.

Here's an example of what I mean:

public static string GetRangeString(int[] arr)
{
    // Return fast if array is null or contains less than 2 items
    if (arr == null || !arr.Any()) return string.Empty;
    if (arr.Length == 1) return arr[0].ToString();

    var rangeString = new StringBuilder();
    bool isRange = false;

    for (int i = 0; i < arr.Length; i++)
    {
        while (i < arr.Length - 1 && arr[i] + 1 == arr[i + 1])
        {
            if (!isRange) rangeString.Append($"{arr[i]}");
            isRange = true;
            i++;
        }

        if (isRange)
        {
            rangeString.Append("-");
            isRange = false;
        }

        rangeString.Append($"{arr[i]},");
    }

    return rangeString.ToString().TrimEnd(',');
}

Usage

static void Main()
{
    var arr = new[] { 1, 5, 2, 3, 4, 6, 8, 9, 10, 11 };
    Console.WriteLine(GetRangeString(arr));

    Console.Write("\nPress any key to exit...");
    Console.ReadKey();
}

Output

Answer 4

Pass in the array. Otherwise this code is simply not useful.

public string[] GetArray(int[] someIntegers)

---

Make an array of range-strings, not a comma-separated-values string. Otherwise there is no point in returning a int[].

---

Sort the original array.

---

Does this code work? Hard coding the array in "success optimized" order hides bugs.

---

Is an array appropriate for your needs? I wonder about converting ToList() smack in the middle of processing. Your data should be in an usable structure from the start and the rest of the code should not have to worry about it.

        var data = z.ToList();
        if (data.Count() == 1)
            return data.First().ToString();