24

I have a class with two readonly int fields. They are exposed as properties:

public class Thing
{
    private readonly int _foo, _bar;

    /// <summary> I AM IMMUTABLE. </summary>
    public Thing(int foo, int bar)
    {
        _foo = foo;
        _bar = bar;
    }

    public int Foo { get { return _foo; } set { } }

    public int Bar { get { return _bar; } set { } }
}

However, that means that the following is perfectly legal code:

Thing iThoughtThisWasMutable = new Thing(1, 42);

iThoughtThisWasMutable.Foo = 99;  // <-- Poor, mistaken developer.
                                  //     He surely has bugs now. :-(

The bugs that come from assuming that would work are sure to be insidious. Sure, the mistaken developer should have read the docs. But that doesn't change the fact that no compile- or run-time error warned him about the problem.

How should the Thing class be changed so that devs are less likely to make the above mistake?

Throw an exception? Use a getter method instead of a property?

kdbanman
  • 1,447

4 Answers4

105

Why make that code legal?
Take out the set { } if it does nothing.
This is how you define a read only public property: 

public int Foo { get { return _foo; } }
paparazzo
  • 1,927
44

With C#5 and before, we were faced with two options for immutable fields exposed via a getter:

  1. Create a read-only backing variable and return that via a manual getter. This option is secure (one must explicitly remove the readonly to destroy the immutability. It created lots of boiler-plate code though.
  2. Use an auto-property, with a private setter. This creates simpler code, but it's easier to accidentally break the immutability.

With C# 6 though (available in VS2015, which was released yesterday), we now get the best of both worlds: read-only auto properties. This allows us to simplify the OP's class to:

public class Thing
{
    /// <summary> I AM IMMUTABLE. </summary>
    public Thing(int foo, int bar)
    {
        Foo = foo;
        Bar = bar;
    }

    public int Foo { get; }
    public int Bar { get; }
}

The Foo = foo and Bar = bar lines are only valid in the constructor (which achieves the robust read-only requirement) and the backing field is implied, rather than having to be explicitly defined (which achieves the simpler code).

David Arno
  • 39,599
  • 9
  • 94
  • 129
12

You could just get rid of the setters. They don't do anything, they will confuse users and they will lead to bugs. However, you could instead make them private and thus get rid of the backing variables, simplifying your code:

public class Thing
{
    public Thing(int foo, int bar)
    {
        Foo = foo;
        Bar = bar;
    }

    public int Foo { get; private set; }

    public int Bar { get; private set; }
}
David Arno
  • 39,599
  • 9
  • 94
  • 129
6

Two solutions.

Simple:

Don't include setters as noted by David for immutable readonly objects.

Alternatively:

Allow setters to return a new immutable object, verbose in comparison to the former but provides state over time for each initialised object. This design is a very useful tool for Thread safety and immutability which extends over all imperative OOP languages.

Pseudocode

public class Thing
{

{readonly vars}

    public Thing(int foo, int bar)
    {
        _foo = foo;
        _bar = bar;
    }

    public Thing withFoo(int foo) {
        return new Thing(foo, getBar());
    }

    public Thing withBar(int bar) {
        return new Thing(getFoo(), bar)
    }

    etc...
}

public static factory

public class Thing
{

{readonly vars}

    private Thing(int foo, int bar)
    {
        _foo = foo;
        _bar = bar;
    }

    public static with(int foo, int bar) {
        return new Thing(foo, bar)
    }

    public Thing withFoo(int foo) {
        return new Thing(foo, getBar());
    }

    public Thing withBar(int bar) {
        return new Thing(getFoo(), bar)
    }

    etc...
}
Matt Smithies
  • 61