2

I'm trying to refactor a method that parses through a file. To support files of arbitrary size, the method using a chunking approach with a fixed buffer.

public int Parse()
{
    // Get the initial chunk of data
    ReadNextChunk();

    while (lengthOfDataInBuffer > 0)
    {
       [parse through contents of buffer]

       if (buffer_is_about_to_underflow)
          ReadNextChunk();
    }

    return result;
}

The pseudo code above is part of the only public non-static method in a class (other than the constructor). The class only exists to encapsulate the state that must be tracked while parsing through a file. Further, once this method has been called on the class, it can't/shouldn't be called again. So the usage pattern looks like this:

var obj = new MyClass(filenameToParse);

var result = obj.Parse();

// Never use 'obj' instance again after this.

This bugs me for some reason. I could make the MyClass constructor private, change Parse to a static method, and have the Parse method new up an instance of Parse scoped to the method. That would yield a usage pattern like the following:

var result = MyClass.Parse(filenameToParse);

MyClass isn't a static class though; I still have to create a local instance in the Parse method.

Since this class only has two methods; Parse and (private) ReadNextChunk, I'm wondering if it might not be cleaner to write Parse as a single static method by embedding the ReadNextChunk logic within Parse as an anonymous method. The rest of the state could be tracked as local variables instead of member variables.

Of course, I could accomplish something similar by making ReadNextChunk a static method, and then passing all of the context in, but I remember that anon methods had access to the outer scope.

Is this weird and ugly, or a reasonable approach?

Scott Smith
  • 3,900
  • 2
  • 31
  • 63
  • Why not anonymous TYPE? – SimpleVar Jul 10 '13 at 01:11
  • If the method is static then how are you going to write unit tests for it? You're tightly coupling the File System to your parser. – Dustin Kingen Jul 10 '13 at 01:25
  • 1
    @Romoku The same way you write unit tests for any other methods. It's only mocking it that's harder with static methods, and if there's never a reason to mock it, that's not a problem. I'd say that creating an instance that doesn't actually hold onto any useful state would be the design flaw; if the method doesn't need any state, it should be static. Also, the title is misleading. There is nothing in the body of the question about an anonymous anything, it's just a static vs instance question. – Servy Jul 10 '13 at 01:28
  • 2
    @Servy He said he could turn `ReadNextChunk` into an anonymous method. – Dustin Kingen Jul 10 '13 at 01:33
  • Here's a [class](https://github.com/scriptcs/scriptcs/blob/master/src/ScriptCs.Core/FilePreProcessor.cs) from ScriptCS that I think resembles your problem. – Dustin Kingen Jul 10 '13 at 01:45
  • @Yorye - Not sure what you mean, can you elaborate? – Scott Smith Jul 10 '13 at 16:25
  • @Romoku - In the actual code, I'm passing in a stream. – Scott Smith Jul 10 '13 at 16:26

3 Answers3

4

This maybe suitable more to code review.

However, these are my comments about your design:

  1. I don't think it will matter much about obj instance only used once. If you bugged with it, there are 2 ways to trick it:

    • Use of another method such as:

      public int Parse()
      {
          var obj = new MyClass(filenameToParse);
          return obj.Parse();
      }
      
    • Make the MyClass implement IDisposable and wrap it in using statement. I don't recommend this since usually IDisposable has logic in their Dispose() method

  2. I think it is better to make your MyClass accept parameter in Parse to Parse(string fileNameToParse). It will make MyClass as a service class, make it stateless, reusable and injectable.

  3. Regarding impact to static class. First it add coupling between your consumer and MyClass. Sometimes if you want to test / unit test the consumer without using the MyClass parser, it will be hard / impossible to mock the MyClass into something you want.

Fendy
  • 4,565
  • 1
  • 20
  • 25
0

All you need is a static parse method that creates an instance, much like what you suggest in your question

public class MyClass
{
     // your existing code.... but make the members and constructor private.


     public static int Parse(string filenameToParse)
     {
         return new MyClass(filenameToParse).Parse();
     }
}

then

just use it like you suggest...

var result = MyClass.Parse(filenameToParse);
Keith Nicholas
  • 43,549
  • 15
  • 93
  • 156
0

MyClass isn't a static class though; I still have to create a local instance in the Parse method.

You don't need a static class to be able to leverage static methods. For example this works fine:

public class MyClass
{
    public static string DoStuff(string input)
    {
        Console.WriteLine("Did stuff: " + input);
        return "Did stuff";
    }
}

public class Host
{
    public void Main()
    {
        MyClass.DoStuff("something");
    }
}
nathanchere
  • 8,008
  • 15
  • 65
  • 86
  • I understand about static methods. My point was that I'm still using an instance of the class (within the static method) to track the state. My question was whether there is an elegant way to call a block of code within a static method, where that code has access to the local variables in the calling method. – Scott Smith Jul 10 '13 at 16:32
  • Why not also have a private static method which takes the variables you want to access as additional parameters, and have the public one call the private one? – nathanchere Jul 10 '13 at 22:20