4
\$\begingroup\$

I'm writing a parser and I need to iterate through text vertically (down to bottom) and horizontally (left to right).

This alone would be easy but some text parts need to be rescanned by another parser and for this I need to reiterate over random lines or a text range.

I wrote two enumerators.

One for enumerating all lines or only some of them:

internal class LineEnumerator
{
    private const int IndexOutOfRange = -1;

    public LineEnumerator(IReadOnlyList<string> lines) : this(lines, null) { }

    public LineEnumerator(IReadOnlyList<string> lines, IReadOnlyList<int> lineNumbers)
    {
        Lines = lines;
        Index = -1;
        LineNumbers = lineNumbers;
    }

    public IReadOnlyList<string> Lines { get; }
    public IReadOnlyList<int> LineNumbers { get; }
    public int Index { get; private set; }
    public bool IsIndexInRange => Index > IndexOutOfRange && Index < (LineNumbers?.Count ?? Lines.Count);
    public int LineNumber
    {
        get
        {
            if (!IsIndexInRange) { return IndexOutOfRange; }
            return LineNumbers?[Index] ?? Index;
        }
    }
    public string Current => IsIndexInRange ? Lines[LineNumber] : null;

    public CharEnumerator Chars => IsIndexInRange ? new CharEnumerator(Current) : null;

    public bool MoveNext() => ++Index < (LineNumbers?.Count ?? Lines.Count);
}

and another one for enumerating chars or only a particular range:

internal class CharEnumerator
{
    private const int IndexOutOfRange = -1;

    public CharEnumerator(string line) : this(line, 0, line.Length) { }

    public CharEnumerator(string line, int index0, int length)
    {
        Line = line;
        Index0 = index0;
        Length = length;
        Index = IndexOutOfRange;
    }

    public string Line { get; }
    public int Index0 { get; }
    public int Length { get; }
    public int Counter { get; private set; }
    public int Index { get; private set; }
    public bool IsIndexInRange => Index >= Index0 && Counter <= Length;
    public char Current => IsIndexInRange ? Line[Index] : '\0';

    public bool MoveNext()
    {
        if (!IsIndexInRange)
        {
            Index = Index0;
            Counter++;
            return true;
        }
        Index++;
        Counter++;
        return (Counter <= Length);
    }
}

They shouldn't just enumerate the text but also keep track of where they are so that I can throw exceptions with exact location if something goes wrong thus the line enumerator needs to work with real indexes even for a range. The same applies to the char enumerator.

I feel like the enumerators violate SRP (single responsibility principle) and I should create two more enumerators for enumerating a range of lines or a range of characters. Is this necessary or would you leave it like that?


Examples

var lines = new[] { "foo", "bar", "baz", "qux" };
var lineEnumerator = new LineEnumerator(lines);
while (lineEnumerator.MoveNext())
{
    Console.WriteLine(lineEnumerator.Current);
}

foo bar baz qux

Console.WriteLine();
var lineRangeEnumerator = new LineEnumerator(lines, new[] { 1, 3 });
while (lineRangeEnumerator.MoveNext())
{
    Console.WriteLine(lineRangeEnumerator.Current);
}

bar qux

Console.WriteLine();
var line = "foo bar baz";
var charEnumerator = new CharEnumerator(line);
while (charEnumerator.MoveNext())
{
    Console.WriteLine(charEnumerator.Current);
}

f o o b a r b a z

Console.WriteLine();
var charRangeEnumerator = new CharEnumerator(line, 4, 3);
while (charRangeEnumerator.MoveNext())
{
    Console.WriteLine(charRangeEnumerator.Current);
}

b a r

\$\endgroup\$
  • \$\begingroup\$ What exactly are you parsing? Does this interface with excel or something else? \$\endgroup\$ – pacmaninbw Aug 7 '16 at 15:07
  • \$\begingroup\$ @pacmaninbw no, it doesn't interface with excel. I'm parsing my own template format. \$\endgroup\$ – t3chb0t Aug 7 '16 at 15:13
3
\$\begingroup\$

I would go with standard IEnumerator and IEnumerable, so it could be consumed as:

    static void Main(string[] args)
    {
        var template = new Template("foo", "bar", "baz", "qux");

        foreach (var row in template)
            Console.WriteLine(row.Text);

        Console.WriteLine();
        foreach (var row in template.Select(1, 3))
            Console.WriteLine(row.Text);

        Console.WriteLine();
        foreach (var row in template.Select(0))
            foreach (var column in row)
                Console.WriteLine(column.Text);

        Console.WriteLine();
        foreach (var row in template.Select(0))
            foreach (var column in row.Range(0,3))
                Console.WriteLine(column.Text);
    }

Where Select allows to subset by indecies provided, while Range is working in terms of start and count. Row and Column objects have Index (current selection) and Indecies (allowed to be selected) properties:

public class Template : IEnumerable<Row>
{
    public Template(params string[] lines)
    {
        Lines = lines;
    }

    public IEnumerable<Row> Range(int start, int count) => Select(Enumerable.Range(start, count));
    public IEnumerable<Row> Select(params int[] indecies) => Select(indecies as IEnumerable<int>);
    public IEnumerable<Row> Select(IEnumerable<int> indecies)
    {
        foreach (var index in indecies)
            yield return new Row(Lines.ElementAtOrDefault(index), index, indecies);
    }

    public IEnumerator<Row> GetEnumerator() => Range(0, Lines.Length).GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
    string[] Lines { get; }
}

And:

public struct Row : IEnumerable<Column>
{
    public Row(string text, int index, IEnumerable<int> indecies)
    {
        Text = text;
        Index = index;
        Indecies = indecies;
    }

    public string Text { get; }
    public int Index { get; }
    public IEnumerable<int> Indecies { get; }

    public IEnumerable<Column> Range(int start, int count) => Select(Enumerable.Range(start, count));
    public IEnumerable<Column> Select(params int[] indecies) => Select(indecies as IEnumerable<int>);
    public IEnumerable<Column> Select(IEnumerable<int> indecies)
    {
        foreach (var index in indecies)
            yield return new Column(Text.ElementAtOrDefault(index), index, indecies);
    }

    public IEnumerator<Column> GetEnumerator() => Range(0, Text.Length).GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

And:

public struct Column
{
    public Column(char text, int index, IEnumerable<int> indecies)
    {
        Text = text;
        Index = index;
        Indecies = indecies;
    }

    public char Text { get; }
    public int Index { get; }
    public IEnumerable<int> Indecies { get; }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Damn! This is agian a nice aproach ;-D \$\endgroup\$ – t3chb0t Aug 7 '16 at 18:38
  • \$\begingroup\$ I'll change the ElementAtOrDefault into return indecies.Select(index => new Row(Lines[index], index, indecies)); because it will definitely exist and if not, this means I made a mistake somewhere else and this will allow me to spot the bug soon enough. \$\endgroup\$ – t3chb0t Aug 7 '16 at 18:55
  • 1
    \$\begingroup\$ Just in case - ElementAt and ElementAtDefault are smart enough to check actual data type interfaces and consume IList<T>[index] if implemented. \$\endgroup\$ – Dmitry Nogin Aug 7 '16 at 19:05
  • \$\begingroup\$ BTW it's "indices" ;) \$\endgroup\$ – 404 Aug 7 '16 at 23:05
1
\$\begingroup\$

Most of my comments have been mentioned by @Dmitry Nogin. In addition

  • In your LineEnumerator constructor, I don't see the reason why you can't assign IndexOutOfRange to Index just like you've done in CharEnumerator constructor
  • Use readonly keyword in place of get{..} since the values for the below fields are assigned only through the constructor
public  readonly string Line;
public readonly int Index0;
public readonly int Length;
  • I don't think you should be passing length as a parameter because you could always retrieve the value from line.Length. A potential problem would be if the user assigned -1 to Length e.g
    var charEnumerator = new CharEnumerator(line,2,-1);.

This line of code will never evaluate to true
public bool IsIndexInRange => Index >= Index0 && Counter <= Length;

  • To be honest, when I saw your two enumerators I expected the classes to implement IEnumerator<T>, IEnumerator interfaces as these interfaces contain members such as MoveNext, Current and Reset() which you have manually defined here.
  • When I passed in these values var charEnumerator = new CharEnumerator(line,2,11); I can't extract the character at that position except when I do this
charEnumerator.MoveNext();
Console.WriteLine(charEnumerator.Current);

In the ideal scenario would have expected the charEnumerator to have a functionality of returning the item rather than doing these two steps above.

  • Just like the other reviewer has suggested using a struct in the scenario is a good approach. I would have done the same myself
  • When you use IReadOnlyList, it's possible the second constructor can be called with null, null as they are valid inputs. Calling any member properties will result in System.NullReferenceException
   var lineEnumerator = new LineEnumerator(null, null);
     Console.WriteLine(lineEnumerator.IsIndexInRange); 

This should be taken into consideration.

I will be back for the LineEnumerator review later today . Cheers

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for the review ;-) I agree with you on some points but on others not, I will comment on the latter... Use readonly keyword I wanted all members to be properties because I thougt it might be better like that, just a prejudice ;-] passing length as a parameter - this is a vital feature because I sometimes need only a part of the string at an exact position. IsIndexInRange - this is indeed true when the enumerator is in the allowed range, the last MoveNext will exceed the count and make if false. CharEnumerator(line,2,11); - this is a vital feature too ;-) \$\endgroup\$ – t3chb0t Aug 10 '16 at 4:09
  • \$\begingroup\$ it's possible the second constructor can be called with null, null - theoretically yes but this would be huge bug in the core as there musn't be any nulls ever (as long as the index/length calculations are fine) ;-] I agree with all other points and I changed it to Dimitri's solution. It now works like a charm. I admit, mine was broken in so many places and extremely difficult to use in some places. \$\endgroup\$ – t3chb0t Aug 10 '16 at 4:15
  • \$\begingroup\$ To be honest , using readonly is a matter of style @t3chb0t . \$\endgroup\$ – Siobhan Aug 10 '16 at 6:17

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.