Create new instance or just set internal variables

Question

I've written a helper class that takes a string in the constructor and provides a lot of Get properties to return various aspects of the string. Currently the only way to set the line is through the constructor and once it is set it cannot be changed. Since this class only has one internal variable (the string) I was wondering if I should keep it this way or should I allow the string to be set as well?

Some example code my help why I'm asking:

StreamReader stream = new StreamReader("ScannedFile.dat");
ScannerLine line = null;
int responses = 0;
while (!stream.EndOfStream)
{
  line = new ScannerLine(stream.ReadLine());
  if (line.IsValid && !line.IsKey && line.HasResponses)
    responses++;
}

Above is a quick example of counting the number of valid responses in a given scanned file. Would it be more advantageous to code it like this instead?

StreamReader stream = new StreamReader("ScannedFile.dat");
ScannerLine line = new ScannerLine();
int responses = 0;
while (!stream.EndOfStream)
{
  line.RawLine = stream.ReadLine();
  if (line.IsValid && !line.IsKey && line.HasResponses)
    responses++;
}

This code is used in the back end of a ASP.net web application and needs to be somewhat responsive. I am aware that this may be a case of premature optimization but I'm coding this for responsiveness on the client side and maintainability.

Thanks!

EDIT - I decided to include the constructor of the class as well (Yes, this is what it really is.) :

public class ScannerLine
{
  private string line;
  public ScannerLine(string line)
  {
    this.line = line;
  }

  /// <summary>Gets the date the exam was scanned.</summary>
  public DateTime ScanDate
  {
    get
    {
      DateTime test = DateTime.MinValue;
      DateTime.TryParseExact(line.Substring(12, 6).Trim(), "MMddyy", CultureInfo.InvariantCulture, DateTimeStyles.None, out test);
      return test;
    }
  }

  /// <summary>Gets a value indicating whether to use raw scoring.</summary>
  public bool UseRaw { get { return (line.Substring(112, 1) == "R" ? true : false); } }

  /// <summary>Gets the raw points per question.</summary>
  public float RawPoints
  {
    get
    {
      float test = float.MinValue;
      float.TryParse(line.Substring(113, 4).Insert(2, "."), out test);
      return test;
    }
  }
}

**EDIT 2 - ** I included some sample properties of the class to help clarify. As you can see, the class takes a fixed string from a scanner and simply makes it easier to break apart the line into more useful chunks. The file is a line delimiated file from a Scantron machine and the only way to parse it is a bunch of string.Substring calls and conversions.

Answer 1

You're first approach is more clear. Performance wise you can gain something but I don't think is worth.

Answer 2

I would go with the second option. It's more efficient, and they're both equally easy to understand IMO. Plus, you probably have no way of knowing how many times those statements in the while loop are going to be called. So who knows? It could be a .01% performance gain, or a 50% performance gain (not likely, but maybe)!

Answer 3

I would definitely stick with the immutable version if you really need the class at all. Immutability makes it easier to reason about your code - if you store a reference to a ScannerLine, it's useful to know that it's not going to change. The performance is almost certain to be insignificant - the IO involved in reading the line is likely to be more significant than creating a new object. If you're really concerned about performance, should should benchmark/profile the code before you decide to make a design decision based on those performance worries.

However, if your state is just a string, are you really providing much benefit over just storing the strings directly and having appropriate methods to analyse them later? Does ScannerLine analyse the string and cache that analysis, or is it really just a bunch of parsing methods?

Answer 4

Immutable classes have a lot of advantages. It makes sense for a simple value class like this to be immutable. The object creation time for classes is small for modern VMs. The way you have it is just fine.

Answer 5

I'd actually ditch the "instance" nature of the class entirely, and use it as a static class, not an instance as you are right now. Every property is entirely independent from each other, EXCEPT for the string used. If these properties were related to each other, and/or there were other "hidden" variables that were set up every time that the string was assigned (pre-processing the properties for example), then there'd be reasons to do it one way or the other with re-assignment, but from what you're doing there, I'd change it to be 100% static methods of the class.

If you insist on having the class be an instance, then for pure performance reasons I'd allow re-assignment of the string, as then the CLR isn't creating and destroying instances of the same class continually (except for the string itself obviously).

At the end of the day, IMO this is something you can really do any way you want since there are no other class instance variables. There may be style reasons to do one or the other, but it'd be hard to be "wrong" when solving that problem. If there were other variables in the class that were set upon construction, then this'd be a whole different issue, but right now, code for what you see as the most clear.

Answer 6

I'd go with your first option. There's no reason for the class to be mutable in your example. Keep it simple unless you actually have a need to make it mutable. If you're really that concerned with performance, then run some performance analysis tests and see what the differences are.