Substring extension method with a method of removal

Question

I consistently have code where I need to pull individual chunks of substrings out of a larger string; while I won't get into details, it's easy to replicate in a proof of concept:

var data = "alja5nerjvnalskjnbaeviunreklvnaslidhfvbaelkjnrfvliasndve";
var open = data.Substring(0, 4);
var inputLength = int.Parse(data.Substring(4, 1));
var input = data.Substring(5, inputLength);
data = data.Substring(5 + inputLength);
...

Now, let's ignore the ellipsis for now and focus on the magic numbers and having to keep track of indices. I'm not a fan of that, I'm sure others aren't either, so a way to make this easier to understand is to remove what we just retrieved from input, each time we retrieve it:

var data = ...;

// Get the open variable, then remove it from the data.
const int openLength = 4;
var open = input.Substring(0, openLength);
data = data.Remove(0, openLength);

// Get the input length variable, then remove it from the data.
const int expectedInputLength = 1; // Bad naming, I know.
var inputLength = int.Parse(data.Substring(0, expectedInputLength));
data = data.Remove(0, expectedInputLength);

// Get the input variable, then remove it from the data.
var input = data.Substring(0, inputLength);
data = data.Remove(0, inputLength);

Unfortunately, this way gets quite verbose and leaves developers leaning towards the original snippet, which is fine, it's just harder to manage (in my opinion). However, I've decided to create an extension method for strings that handles this use case in a more efficient way:

public static string Substring(this string input, int startingIndex, int length, ref string removeFrom) {
    var substring = input.Substring(startingIndex, length);
    removeFrom = removeFrom.Remove(startingIndex, length);
    return substring;
}

The usage changes the previously verbose code back to the original snippet's form, with less guess work:

var data = ...;
var open = data.Substring(0, 4, removeFrom: ref data);
var inputLength = int.Parse(data.Substring(0, 1), removeFrom: ref data));
var input = data.Substring(0, inputLength, removeFrom: ref data);

Working Example

I've created a working example that you can test on .NET Fiddle; the complete code for it is:

using System;
public static class ExtensionMethods {
    public static string Substring(this string input, int startingIndex, int length, ref string removeFrom) {
        var substring = input.Substring(startingIndex, length);
        removeFrom = removeFrom.Remove(startingIndex, length);
        return substring;
    }   
}
public class Program {
    public static void Main() {
        var data = "alja5nerjvnalskjnbaeviunreklvnaslidhfvbaelkjnrfvliasndve";
        Console.WriteLine(data);
        
        var open = data.Substring(0, 4, removeFrom: ref data);
        var inputLength = int.Parse(data.Substring(0, 1, removeFrom: ref data));
        var input = data.Substring(0, inputLength, removeFrom: ref data);
        
        Console.WriteLine(open);
        Console.WriteLine(inputLength);
        Console.WriteLine(input);
        Console.WriteLine(data);
    }
}

The expected output of which is:

alja5nerjvnalskjnbaeviunreklvnaslidhfvbaelkjnrfvliasndve
alja
5
nerjv
nalskjnbaeviunreklvnaslidhfvbaelkjnrfvliasndve

Notes

I have no interest in discussing the following:

• The use of Parse over TryParse.
  • I don't care if the proof of concept blows up, I use TryParse where necessary.
• The use of magic numbers in the second and final examples.
  • I will actually have constants to represent things such as openLength and expectedInputLength.
• The lack of validation for removeFrom in the extension method.
  • I want this to blow up if the implementation doesn't check the value prior.
• Stylistic issues such as:
  • Brace placement.
  • Indentation.

---

I'd like the community's review of this extension method. My biggest concern in particular is the name of the method not clearly articulating what's going on, even with the named variable in the implementation (which would be a standards thing).

Answer 1

Reusability vs readability

You've definitely tried to improved the code and abstract it. You've made some strides in promoting reusability.

However, you haven't tackled the bigger issue of readability and complicated code (e.g. index juggling), which make both the old and new examples cumbersome to handle. Additionally, the signature of your fixes is similarly complicated, therefore compounding the issue of complexity.

The main issue here is ref data. Having to pass the same data reference twice really detracts from your solution. Sadly, you can't alter a string by reference by doing myString.MyMethod(). I understand why you therefore started using ref data as this does allow you to alter by reference, but this is a case where you should've looked for alternatives, instead of persisting with your initial idea. It's all about finding the path of least resistance, and your solution met more resistance than it could have.

Secondly, it's actually inefficient to always create new strings. Strings are immutable, and each new string (= unique sequence of characters) you create takes up more and more memory. The garbage collector tries to catch up but you can create significant memory issues if your logic very quickly generates many new strings.

I want to redesign the approach from the ground up, as this will help in understanding how to alleviate the complexities and making sure that the abstractions we implement actually make the end user's life simpler.

I suggest you compare your approach with my suggested approach, both from the perspective of the developer who created them and the developer who will use them. Which is cleaner? Which do you think a newcomer will understand better? Why? How you could have changed one approach to be more like the other?

---

Basic solution

What you're really trying to do, when all it said and done, it to take a certain amount of characters from a string, in a way that it also removes them from the original. That is the outset, and we should design our solution to best fit with this goal.

As I mentioned before, it's actually inefficient to always create new strings. It's better to keep working with the same string, and just have a secretly tracked index which automatically skips the first X characters of the string (i.e. the characters you've already handled).

Because you are trying to track state (i.e. the secret index), an OOP solution will help you encapsulate your state and use it while at the same time hiding it from the consumer.
A second way you could've come to the realization that OOP is useful here is that you can't alter a string by reference by doing myString.MyMethod(), but if you wrap that string in an object, you can alter that object by reference by doing myObjectContainingMyString.MyMethod().

To the consumer, this alternate approach will handle exactly the same. It's as if the handled characters no longer exist. But internally, you simply skip over the characters. They're still there, they're just not used anymore.

I'm going to call this a StringQueue, because it basically makes your data behave like a queue of substrings.

First, the initial state. We create a queue, with a given data string. The secret index is 0 since we have not processed any characters yet.

public class StringQueue
{
    private readonly string original;
    private int index;

    public StringQueue(string data)
    {
        this.original = data;
        this.index = 0;
    }
}

Now we want to have a public method that allows you to take the next X characters. The secret index then also increases by X.

public string Next(int count)
{
    var chars = original.Substring(index, count);
    index += count;
    
    return chars;
}

However, I want to suggest a readability improvement here. LINQ adds a lot of nice and readable features to C#, allowing you to cut down on the syntactic complexity and keeps things readable.
As a string is inherently treated as if it's a char[], this means you can rther easily rewrite the above method to be nicer to read:

public string Next(int count)
{
    var chars = original.Skip(index).Take(count);
    index += count;
    
    return new string(chars.ToArray());
}

Does it matter much? Well, for this small method, maybe it doesn't. Maybe the (arguably negligible in most cases) performance difference matters more to you. That is certainly a possibility.

I find the LINQ approach to be more intuitively understandable. I don't use Substring much and I sometimes forget whether the second parameter denotes a length or an ending index. But Skip and Take are clear and unambiguous.

Even if you don't use it here, LINQ is a good trick to keep in your repertoire because when the code gets to be more complex, the readability enhancement can do a lot to help keep things sane and readable.

Basic usage of StringQueue would be (fiddle)

var q = new StringQueue("abcdefghij");

for(int i = 0; i < 5; i++)
{
    Console.WriteLine(q.Next(3));
}

Output:

abc
def
ghi
j       // 1 character
        // empty string

This has significantly whittled down the complexity of the syntax when using the StringQueue. All you need to do now is specify the source string and tell it how big your chunks should be.

---

Further improvement

There's another feature you could add here. I'm unsure if this is needed for your current use case, but it might be nicer to use in certain scenarios where you break a string in many chunks at the same time.

By implementing method chaining and out parameters, you could create a syntax that allows you to very quickly define your chunks.

public StringQueue Next(int count, out string result)
{
    result = Next(count);
    return this;
}

Note that this relies on the existence of the earlier Next method we discussed.

This allows for a syntax using out parameters and quick chaining:

new StringQueue("abcdefghij")
    .Next(3, out string first)
    .Next(3, out string second)
    .Next(3, out string third);
    
Console.WriteLine(first);
Console.WriteLine(second);
Console.WriteLine(third);

Output:

abc
def
ghi

I really like this approach. The one niggle here is that it doesn't play nice with converting the substrings to different types, because that would break the chain. But we can add this functionality.

Note that I'm intentionally not implementing the conversion logic for specific target types in StringQueue itself, as this would violate SRP. It is up to the consumer to decide how they want their substrings to be converted.

public T Next<T>(int count, Func<string, T> convert)
{
    return convert(Next(count));
}

public StringQueue Next<T>(int count, out T result, Func<string, T> convert)
{
    result = Next(count, convert);
    return this;
}

Usage:

// With method chaining

new StringQueue("abc123ghij")
    .Next(3, out string first)
    .Next(3, out int second, s => Convert.ToInt32(s))
    .Next(3, out string third);
    
Console.WriteLine(first);
Console.WriteLine(second);
Console.WriteLine(third);

// Without method chaining

var q = new StringQueue("abc123ghij");
    
Console.WriteLine(q.Next(3));
Console.WriteLine(q.Next(3, s => Convert.ToInt32(s)));
Console.WriteLine(q.Next(3));

---

All code and usage examples can be found in this fiddle.

Answer 2

Overloading a built-in string method with an extension method is not a good idea IMO, especially something as well known as Substring.

I'd also avoid mutating arguments, it's a recipe for confusing code. You do have the ref clue here but it's a practice best avoided. It's easier to reason about code that takes an input and returns an output than code that takes an input, changes it and also returns an output.

The simple answer to your question is to create a static helper:

public static (string substring, string remaining) Cut(string input, int start, int length)
{
    var substring = input.Substring(start, length);
    var remaining = input.Remove(start, length);
    return (substring, remaining);
}

You can keep the ref if that's your thing:

public static string Cut(ref string input, int startingIndex, int length) 
{
    var substring = input.Substring(startingIndex, length);
    input = input.Remove(startingIndex, length);
    return substring;
}

I think the better answer would be to create a class to hold the parsing logic. Consuming it could look like this:

var data = ...
var parsed = MyParser.Parse(data);
Console.WriteLine(parsed.Open);
Console.WriteLine(parsed.OtherStuff);

It means your MyParser can also own all of the magic numbers.

Answer 3

Let me share with you yet another solution. ;)

The basic idea is the following:

• Define slices (upfront) by specifying their length and optional padding
• Iterate through the slices and return chunks after each other
• Use Ranges instead of the substring method

So, first let's define the Slice:

struct Slice
{
    public byte Length { get; init; }
    public byte? Padding { get; init; }
}

• It could be a class or a record it really does not matter

Next, let's define the slices upfront:

var slices = new List<Slice>
{
    new Slice { Length = 4 },
    new Slice { Length = 1 },
    new Slice { Length = 5 },
    new Slice { Length = 6, Padding = 1 },
};

Now let's implement the iterable method which returns the chunks:

public static IEnumerable<string> GetChunks(string input, IEnumerable<Slice> slices)
{
    byte currentPadding = 0;
    foreach (var slice in slices)
    {
        int startindex = currentPadding + (slice.Padding ?? 0);
        var chunk = new string(input.AsSpan()[(startindex)..(startindex+slice.Length)]);
        currentPadding += slice.Length;
        yield return chunk;
    }
    yield return new string(input.AsSpan()[(currentPadding+1)..]);
}

Several notes about the above code:

• For the sake of simplicity I've omitted all safe checks
• currentPadding captures the start position of the next chunk
• if the next Slice defines a padding then we add that to the currentPadding otherwise we add zero
• We treat the string as an ReadOnlySpan<char> to be able to use range-based indexing
• We construct each and everytime a new string based on the slice
• We increment the currentPadding for the next slice and then we yield return the newly constructed string
• And finally we yield return the rest of the string if there is no more slice defined

Please note that we can't preserve the result of input.AsSpan() into a method level variable, because IReadOnlySpan can't be used in an iterator block (CS4013).

Finally, we can simple wire up things:

var data = "alja5nerjvnalskjnbaeviunreklvnaslidhfvbaelkjnrfvliasndve";
foreach (var chunk in GetChunks(data, slices))
{
    Console.WriteLine(chunk);
}

The output will be the following:

alja
5
nerjv
alskjn
baeviunreklvnaslidhfvbaelkjnrfvliasndve

---

Known limitation of this solution: The slices are defined upfront. So, you can't define slices based on the read values.

Answer 4

Here are a couple of observations.

There's a fine balance between elegant code, fast code, and maintainable code.

Although your code may be clean and elegant, even fast, it may not be easy to understand what it actually does at first glance. (i.e. not easily maintainable) That ref removeFrom may be legal and perfectly legitimate as far as the compiler cares, but it takes a minute to wrap one's head around what's happening there.

Also, as others have mentioned, overloading a base method is only sometimes ok, for example if one is making some developer oriented framework / extension. However, be sure to respect the basic intention of the original method. In this case, you are not only performing a substring, but really altering the string that was provided by extracting specific pieces based on some internal logic. IMO, that would warrant a brand new method name that tells the next developer down the line who inherits your code exactly what happens, and what is expected to pop out of the method.

Then you will be free to remove the ref and simply return the modified / desired portion of the string as a return value, instead of altering the original string.

I do like the idea of making some sort of "extract" extension from string as a quick and common way to get some logical portion of the string. I'm surprised there's not a built in way after all these years!

Answer 5

I also like Flater's idea of StringQueue :-)

Here are some additional ideas that you can play with.

Microsoft.Extensions.Primitives has a StringSegment class.

Consider changing the Next function to the following:

public StringSegment Next(int count)
{
    var segment = new StringSegment(data, index, count);
    index += count;
    return segment;
}

For some applications, it's more optimal to work with the underlying char array than the string itself.

Consider creating something like this:

class StringArray
{
    public StringArray(String text)
    {
        data = text.ToCharArray();
    }

    public ArraySegment<char> Next(int count)
    {
        var segment = new ArraySegment<char>(data, index, count);
        index += count;
        return segment;
    }

    private int index;
    private char[] data;
}

And creating functions like the following:

int SegmentToNumber(ArraySegment<char> segment)
{
    int value = 0;
    foreach (char c in segment)
    {
        value *= 10;
        value += c - '0';
    }
    return value;
}

Will any of this make a difference?

I was not really sure myself but decided to do a little testing:

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.Extensions.Primitives;

public class Program
{
    const int numberLength = 5;

    public static int TestStringSimple(List<string> list)
    {
        int total = 0;
        foreach(var line in list)
        {
            int count = line.Length / numberLength;
            for (int i=0; i<count; i++)
            {
                total += int.Parse(line.Substring(i * numberLength, numberLength));
            }
        }
        return total;
    }

    public static int TestStringSegment(List<string> list)
    {
        int total = 0;
        foreach (var line in list)
        {
            int count = line.Length / numberLength;
            for (int i = 0; i < count; i++)
            {
                var segment = new StringSegment(line, i * numberLength, numberLength);
                total += int.Parse(segment);
            }
        }
        return total;
    }

    public static int TestArraySegment(List<string> list)
    {
        int total = 0;
        foreach (var line in list)
        {
            char[] data = line.ToCharArray();
            int count = line.Length / numberLength;
            for (int i = 0; i < count; i++)
            {
                var segment = new ArraySegment<char>(data, i * numberLength, numberLength);
                int value = 0;
                foreach (char c in segment)
                {
                    value *= 10;
                    value += c - '0';
                }
                total += value;
            }
        }
        return total;
    }

    public static string GenerateRandomNumberString(Random random, int length)
    {
        const string chars = "0123456789";
        return new string(Enumerable.Repeat(chars, length)
          .Select(s => s[random.Next(s.Length)]).ToArray());
    }

    public static void Main()
    {
        var rand = new Random();

        int count = 1000000;
        var list = new List<string>();
        for (int i = 0; i < count; i++)
            list.Add(GenerateRandomNumberString(rand, numberLength * 20));

        for (int i = 0; i < 20; i++)
        {
            int total = TestStringSimple(list);
            Console.WriteLine(total);

            total = TestStringSegment(list);
            Console.WriteLine(total);

            total = TestArraySegment(list);
            Console.WriteLine(total);
        }
    }
}

And here are the results:

TestArraySegment has an unfair advantage over the other functions that are using int.Parse since it assumes that all the characters will be digits. If we have more secret knowledge, such as the length of the string, we could create something nasty like the following:

public static int TestArrayNumFive(List<string> list)
{
    int total = 0;
    foreach (var line in list)
    {
        char[] data = line.ToCharArray();
        int count = line.Length / numberLength;
        for (int i = 0; i < count; i++)
        {
            var segment = new ArraySegment<char>(data, i * numberLength, numberLength);
            total += (segment[0] - '0') * 10000 +
                     (segment[1] - '0') * 1000 +
                     (segment[2] - '0') * 100 +
                     (segment[3] - '0') * 10 +
                     (segment[4] - '0');
        }
    }
    return total;
}

That really flies =)