4
\$\begingroup\$

I wanted to parse lines of text as tab-delimited items, and I had just been using a StringReader for something else and I came up with this:

class TabDelimitedFieldReader : StringReader
    {
        private string _nextLine;
        public TabDelimitedFieldReader(string s)
            : base(s)
        {
        }

        public IEnumerable<string> ReadFields()
        {
            if (_nextLine != null)
            {
                var fields = _nextLine.Split('\t');
                foreach (string field in fields)
                {
                    yield return field;
                }
            }
        }

        public bool HasMore
        {
            get
            {
                _nextLine = this.ReadLine();
                return (_nextLine != null);
            }
        }
    }

I can't think of another time I directly subclasses a .NET framework class like this. I figure maybe I'm just strange and it's normal for others, or maybe there's a reason I wouldn't want to do it.

\$\endgroup\$
  • \$\begingroup\$ Not a bad idea. How do you use it, however? Say, I wanted to get text out of clipboard which got pasted in Excel. Then, I would want to make sure that row length is consistent - something that an enumerator would not give me. I would also add an optional bool flag to the ReadFields function which would cause the fields to be trimmed if true. I would run StyleCop on this, which would force you to add comments and rename _nextLine to nextLine, and refer to it as this.nextLine, etc. \$\endgroup\$ – Leonid Feb 9 '12 at 17:45
  • \$\begingroup\$ The context was an ad-hoc report loader service that aggregated data from a list of databases, which I copied/pasted from excel data formatted as a table, so even blanks will generate a blank field, and rows all the same length. Really, style cop would make me use "this" everywhere? I just changed my ambition to work at MS :) \$\endgroup\$ – Aaron Anodide Feb 9 '12 at 17:49
  • \$\begingroup\$ You can turn off the rules that you do not agree with. \$\endgroup\$ – Leonid Feb 9 '12 at 19:36
  • \$\begingroup\$ actually i decided to code the rest of the day using no underscores and "this", and I realized it has a sort of leveling effect that lets me move code in/out/around namespaces/classes/nested classes more freely, so I think I'll stick with it \$\endgroup\$ – Aaron Anodide Feb 10 '12 at 0:36
3
\$\begingroup\$

Conceptually, I do not see anything inherently wrong with subclassing a framework object, particularly since many of them provide virtual/abstract members specifically to allow it.

In this case, it may be better to wrap a TextReader object instead of inheriting from StringReader. This would allow you to provide tab delimited reading for multiple data sources instead of limiting yourself to in-memory strings. For example, you could still pass in a StringReader for reading strings, but you could also pass in a StreamReader if you wanted to support reading from files instead.

Additionally, I noticed that HasMore is dangerous to call due to its side-effect. It is not immediately obvious to a caller that checking HasMore would alter the reader's current position. I would suggest using Peek instead.

You would then need to update _nextLine elsewhere. You could have it be a side-effect of calling ReadFields. Or, perhaps better, you could add another method (e.g., AdvanceLine) that is simply responsible for moving the reader to the next line.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback - question: are you a TDD developer? I'm not but have been lightly using/learning it for a while, and from my limited understanding, code that works is the goal and pre-empting the future is frowned upon... which goes against my instincts too, but seems to be the primary directive (as I understand it). \$\endgroup\$ – Aaron Anodide Feb 9 '12 at 18:00
  • 2
    \$\begingroup\$ By wrapping the class, you are actually reducing the test surface area, as you only need test your externally visible (public/protected) members. By extending StringReader, you would be stuck testing for cases where callers use the public base class methods to ensure you properly handle side-effects. For example, if someone called ReadLine (from TextReader), it would skip a line, changing expected behavior for either ReadFields or HasMore (or both!). \$\endgroup\$ – Dan Lyons Feb 10 '12 at 17:53
  • \$\begingroup\$ @AaronAnodide it's called the decorator pattern. All stream in .NET work in this way. For example you can create a MemoryStream from a FileStream. \$\endgroup\$ – t3chb0t Nov 11 '16 at 13:54

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.