Acceptable way to set readonly field outside of a constructor

Question

I have a constructor that performs initialization on a switch like this:

class Foo {
    public readonly int Bar; 
    public readonly object Baz; 

    public Foo(int bar, string baz) { 
        this.Bar = bar; 
        switch (bar) { 
        case 1: 
            // Boatload of initialization code
            this.Bar = /* value based upon initialization code */
            this.Baz = /* different value based upon initialization code */
        case 2:
            // Different boatload of initialization code
            this.Bar = /* value based upon initialization code */
            this.Baz = /* different value based upon initialization code */
        case 3: 
            // Yet another...
            this.Bar = /* value based upon initialization code */
            this.Baz = /* different value based upon initialization code */ 
        default: 
            // handle unexpected value 
        } 
    }
}

I'm still implementing this, but once done it will easily be a few hundred lines. I'm not a fan of having a constructor this large, but I"m at a loss as to how to either safely bypass this language feature (and bypassing at all is something I don't want to do). Maybe should should be a hint that there's something fundamentally wrong with what I'm trying to do, but I'm not sure.

Basically, I want to perform complex initialization in my own custom immutable type. What's the best way to do this? Is a gazillion line count constructor a terrible thing in this case?

Update: Just for clarification, what I am wanting to do is maintain immutability in a class that would have instances initialized in a complex manner in the best manner possible. I am writing a class that represents a randomly generated token, FormatToken, which would usually be a character.

The complex initialization is parsing a format string (note, I am not trying to parse a regexp to generate a random string, I don't feel like spending my next 20 lifetimes doing this :) ). I was initially writing up something that would accept input through a constructor parameter, such as

+        /// Format tokens
+        /// c{l} Lowercase Roman character in the ASCII range. 
+        /// v{L} Uppercase Roman character in the ASCII range. 
+        /// c Roman character in the ASCII range.
+        /// d Decimal.
+        /// d{0-9} Decimal with optional range, both minimum and maximum inclusive.    

var rand = new RandomString("c{l}C{L}ddd{0-4}d{5-9}"); 
rand.Value == /* could equal "fz8318" or "dP8945", but not "f92781". 

The class that ultimately spawned this question was that which represents each of those tokens. The initialization question comes from being able to support various formats (ASCII characters, roman alphabet, decimals, symbols, etc.)

This is the actual code in question:

internal class FormatToken {
    public TokenType Specifier { get; private set; }  
    public object Parameter { get; private set; }  

    public FormatToken(TokenType _specifier, string _parameter) { 
        // discussion of this constructor at 
        // http://stackoverflow.com/questions/19288131/acceptable-way-to-set-readonly-field-outside-of-a-constructor/
        Specifier = _specifier; 
        _init(_specifier, _parameter); 
    }

    private void _init(TokenType _specifier, string _parameter) { 
        switch (_specifier) { 
        case TokenType.Decimal:
            _initDecimalToken(_parameter); 
            break;
        case TokenType.Literal:
            Parameter = _parameter; 
            break; 
        case TokenType.Roman:
        case TokenType.LowerRoman:
        case TokenType.UpperRoman:
            _initRomanToken(_specifier, _parameter); 
            break;
        default: 
            throw new ArgumentOutOfRangeException("Unexpected value of TokenType."); 
        }
    }

I used readonly initially because I misunderstood the reason to use it. Simply removing readonly and replacing with an auto-property (i.e. { get; private set; } would take care of my immutability concern.

This question has become more a question about the initialization tasks and less about the immutability of FormatToken. Perhaps 'How to perform complex, possibly unknown initialization' is a better question title now. It's now completely obvious to me that having a giant switch is a bad idea. The factory pattern is certainly intriguing for what I'm doing, and I think answers the question I have. I just want to give it a couple of more days.

Thank you so much for your thoughts so far! I'm leaving the initial example code in here to keep the answers making sense.

Answer 1

You might consider using a readonly field storing a mutable struct. Why? Let's boil this down to essentials:

• You want to mutate and shuffle around values during construction. In particular, you want to use normal encapsulation and code reuse techniques such as plain old method calls while constructing your values.
• After construction, you want the value to be fixed.

Structs are essentially just a bag of values; so they easily allow mutation and encapsulation of that mutation during construction. However since they're just a value, they use whatever storage sematics their container provides. In particular, once you store the struct (value) in your readonly field that value can't be mutated (outside of the constructor). Even the struct's own methods cannot mutate non-readonly fields if the struct itself is stored in a readonly field.

For example (pastable in LINQpad):

void Main() {
    MyImmutable o = new MyImmutable(new MyMutable { Message = "hello!", A = 2});
    Console.WriteLine(o.Value.A);//prints 3
    o.Value.IncrementA();        //compiles & runs, but mutates a copy
    Console.WriteLine(o.Value.A);//prints 3 (prints 4 when Value isn't readonly)
    //o.Value.B = 42;            //this would cause a compiler error.
    //Consume(ref o.Value.B);    //this also causes a compiler error.
}
struct MyMutable {
    public string Message;
    public int A, B, C, D;
    //avoid mutating members such as the following:
    public void IncrementA() { A++; } //safe, valid, but really confusing...
}
class MyImmutable{
    public readonly MyMutable Value;
    public MyImmutable(MyMutable val) {
        this.Value=val;
        Value.IncrementA();
    }
}
void Consume(ref int variable){}

The advantage of this technique is that you can have lots of fields and nicely decomposed mutation logic, but nevertheless easily fix the value once it's complete. It also makes copies and copies with minor variations very easy:

var v2 = o.Value;
v2.D = 42;
var d = new MyImmutable(v2);

The downside is that C# mutable structs are unusual and sometimes surprising. If your initialization logic gets complex, you'll be working with parameters and return values with copy semantics, and that's sufficiently different that you may introduce bugs by accident. In particular behavior like that IncrementA() (which changes behavior depending on whether the struct is in a mutable or immutable context) can be subtle and surprising. To stay sane, keep structs simple: avoid methods and properties, and never mutate the contents of the struct in a member.

Answer 2

I think Thomas's approach is the simplest and maintains the constructor API that jdphenix already has.

An alternative approach is to use Lazy to actually defer the setup to when the values are used. I like using Lazy when constructors aren't extremely trivial because 1) setup logic for variables that are never used is never executed and 2) it ensures that creating objects is never surprisingly slow.

In this case, I don't think the setup logic is complex or slow, benefit 1 is really noticeable as a class grows larger and more complex.

class Foo {
    public readonly Lazy<int> Bar; 
    public readonly Lazy<object> Baz; 

    public Foo(int bar, string baz) { 
        this.Bar = new Lazy<int>(() => this.InitBar(bar));
        this.Baz = new Lazy<object>(() => this.InitBaz(bar));
    }

    private int InitBar(int bar)
    {
        switch (bar) { 
        case 1: 
            // Bar for case 1
        case 2:
            // Bar for case 2
        case 3: 
            // etc..
        default: 
        }
    }

    private object InitBaz(int bar)
    {
        switch (bar) { 
        case 1: 
            // Baz for case 1
        case 2:
            // Baz for case 2
        case 3: 
            // etc..
        default: 
        }
    }
}

Answer 3

I would also rather go with Nahum's answer as one of the SOLID principle Open/closed principle will not the be achievable with Switch statements if you want to extend the behavior which is one part. Another part to answer is how to tackle this problem . This can be done by going with inheritance approach and via a Factory method (http://en.wikipedia.org/wiki/Factory_method_pattern) to create an appropriate instance and do a lazy initialization (http://en.wikipedia.org/wiki/Lazy_initialization) of members.

    class FooFactory
    {
        static Foo CreateFoo(int bar,string baz)
        {
              if(baz == "a")
                  return new Foo1(bar,baz);
              else if(baz == "b")
                  return new Foo2(bar,baz);
              ........
        }
    }

    abstract class Foo
    {
          public int bar{get;protected set;}
          public string baz{get;protected set;}
          //this method will be overriden by all the derived class to do
          //the initialization
          abstract void Initialize();
    }

Let the Foo1 and Foo2 derive from Foo and override the Initialize method to provide the appropriate implementation. Since we need to do initialization first for the other methods in Foo to work we can have a bool variable set to true in Initalize method and in other methods we can check whether this value is set to true otherwise we can throw an exception indicating the object needs to be initialized by calling the Initialize method.

Now the client code will look something like this.

   Foo obj = FooFactory.CreateFoo(1,"a");
   obj.Initialize();
   //now we can do any operation with Foo object.

A problem which will occur if we use static method inside a class is that , these methods cant access instance members if needed. So this is where instead of static methods inside the same class we can separate it out as a Factory method to create an instance (but yeah though Singleton works this way , i am more emphasizing this behavior for the current behavior mentioned here because it accesses other appropriate static methods to do its job).

Answer 4

Following up on rasmusgreve and Jon Skeet:

class Foo
{
  public readonly int Bar; 
  public readonly object Baz; 

  private Foo(int bar, string baz) { 
      this.Bar = bar; 
      this.Baz = baz;
  }

  private static Foo _initDecimalToken(string _parameter)
  {
    int calculatedint = 0;
    string calculatedstring = _parameter;
    //do calculations
    return new Foo(calculatedint, calculatedstring);
  }
  private static Foo _initRomanToken(int bar, string _parameter)
  {
    int calculatedint = 0;
    string calculatedstring = _parameter;
    //do calculations
    return new Foo(calculatedint, calculatedstring);
  }
  public static Foo CreateFoo(int bar, string baz)
  {
    switch (bar) 
    { 
      case 1:
        return _initDecimalToken(baz);
      case 2:
        return _initRomanToken(bar, baz);
      default: 
        // handle unexpected value...
        return null;
    }
  }
}

If you wanted to keep Foo lightweight you could put the static construction functions into a separate class (e.g. FooMaker.)

Answer 5

Perhaps I miss the point, but what do you think of :

class Foo
{
    public readonly int Bar;
    public readonly object Baz;

    public Foo(int bar, string baz) { 
        this.Bar = GetInitBar(bar); 
    }

    private int GetInitBar(int bar)
    {
        int result;
         switch (bar) { 
            case 1: 
                // Boatload of initialization code
                result = /* value based upon initialization code */
                result = /* different value based upon initialization code */
            case 2:
                // Different boatload of initialization code
                result = /* value based upon initialization code */
                result = /* different value based upon initialization code */
            case 3: 
                // Yet another...
                result = /* value based upon initialization code */
                result = /* different value based upon initialization code */ 
            default: 
                // handle unexpected value 
        }
        return result;
    }
}

Answer 6

You could use a static factory method on the Foo class in combination with a private constructor. The factory method should be responsible of doing your large switch figuring out the desired values of Bar and Baz, and then simply passing the computed values to the private constructor.

This, of course, does not get rid of the giant switch, but it moves it completely out of the constructor, in which we usually are told that it is bad to do large computations.

This way you end up with something like

class Foo {
    public readonly int Bar; 
    public readonly object Baz; 

    private Foo(int bar, string baz) { 
        this.Bar = bar; 
        this.Bas = baz;
    }

    public static Foo CreateFoo(int bar, string baz)
    {
        int tbar;
        string tbaz;
        switch (bar) { 
        case 1: 
            // Boatload of initialization code
            tbar = /* value based upon initialization code */
            tbaz = /* different value based upon initialization code */
        case 2:
            // Different boatload of initialization code
            tbar = /* value based upon initialization code */
            tbaz = /* different value based upon initialization code */
        //...
        default: 
            // handle unexpected value 
        }
        return new Foo(tbar, tbaz);
    }
}

Answer 7

switch statements are fundamently wrong!

see for explanation: https://softwareengineering.stackexchange.com/questions/147214/refactoring-switch-statements-and-is-there-any-real-use-for-switch-statements-at

here is one way to go: (base class) http://simpleprogrammer.com/2012/02/21/refactoring-switches-to-classes/

here is other way to go: (factory) https://softwareengineering.stackexchange.com/questions/147214/refactoring-switch-statements-and-is-there-any-real-use-for-switch-statements-at

Answer 8

You can use auto-properties:

public int Bar { get; private set; }. You are already capitalizing Bar like if it's a property. Other classes can get Bar, but only your class can set Bar due to its private set; setter.

However, you can set the value of Bar multiple times for each object.

You can set auto-properties in the methods, (but can't use readonly) if you do the constructor Micha's way (https://stackoverflow.com/a/19288211/303939).

Answer 9

If there is anything fundamentally wrong is hard to tell without more informations, but I look not completely wrong (with the showed facts). I would do every case i a own method or maybe with own objects (depends form content). Of course for this you can't use readonly, but Properties with public int Bar { get; private set; } and public object Baz { get; private set; }.

public Foo(int bar, string baz) { 
     this.Bar = bar; 
     switch (bar) { 
        case 1: 
            methodFoo();
        case 2:
            methodBar();
        case 3: 
            methodFooBar();
        default: 
            ExceptionHandling();
}