0

I have an enum State and a class Neighborhood that uses them as data. I'm new to C#, so I'm not sure if this is idiomatic or could be more concise (or just plain wrong).

public enum State : byte
{
    zero = 0,
    one = 1,
    two = 2
}

public class Neighborhood
{

    private State _left, _center, _right;

    public Neighborhood(State left, State center, State right)
    {
        _left = left;
        _center = center;
        _right = right;
    }

    public State left { get { return _left; } }

    public State center { get { return _center; } }

    public State right { get { return _right; } }
}

Is there a shorter or more idiomatic way to do this?

kdbanman
  • 9,073
  • 8
  • 40
  • 74
  • 2
    I'm voting to close this question as off-topic because there is nothing wrong with the code. Questions asking about review or improvements to code belong on codereview.stackexchange.com – Peter Duniho Jul 09 '15 at 04:03
  • 1
    Note that unless the fields are marked with `readonly`, your class is not truly immutable. – Peter Duniho Jul 09 '15 at 04:03
  • spot on. but i supose that Neighborhood could be a struct. – RadioSpace Jul 09 '15 at 04:03
  • 2
    Well, there *is* something wrong with the code, with all the properties returning the value of `_left`... :) – jlahd Jul 09 '15 at 04:04
  • @jlahd: hah! yes, good point. But the OP isn't complaining about any problems related to that, so that _must_ have been on purpose. ;) – Peter Duniho Jul 09 '15 at 04:21
  • Ha! Thanks @jlahd, edited. – kdbanman Jul 09 '15 at 04:45
  • @PeterDuniho, how do I properly migrate the question to CodeReview? When that happens, can you please add your `readonly` idea as an answer? I've never seen that construct before. – kdbanman Jul 09 '15 at 04:47
  • 1
    I think that migration requires moderator intervention (i.e. a flag). Of course, you can always do it manually, simply by copy/pasting the question to the other site and deleting the question here. See [readonly (C# Reference)](https://msdn.microsoft.com/en-us/library/acdd6hb7.aspx) for information on how to use the `readonly` keyword and what it does. – Peter Duniho Jul 09 '15 at 04:50
  • 2
    @kdbanman I don't think this is the greatest post to migrate over to CR. Code Review works best for when you have real working code from an actual project that you want to have feedback on any and all aspects of the code. This question is really just asking "what's the best practice regarding X?", which is explicitly off-topic on CR (see [the help center](http://www.codereview.stackexchange.com/help/on-topic)). – Mathieu Guindon Jul 09 '15 at 04:53
  • @Mat'sMug, alright, then I'll leave this one here. – kdbanman Jul 09 '15 at 05:02

2 Answers2

3
public enum State : byte
{
    zero = 0,
    one = 1,
    two = 2
}

public class Neighborhood
{
    public Neighborhood(State left, State center, State right)
    {
        this.Left = left;
        this.Center = center;
        this.Right = right;
    }

    public State Left { get; private set; }

    public State Center { get; private set; }

    public State Right { get; private set; }
}
Emond Erno
  • 48,121
  • 11
  • 77
  • 106
3

This might not be the most concise way of writing it, but there are a number of differences between using readonly backing fields and private set auto-properties:

  • private set implies that the class implementation can alter the value of the property.
  • readonly backing field can only be initialized from the constructor; the class implementation cannot alter its value.
  • readonly backing field better communicates the intent, if that intent is to make something immutable.
  • backing fields with a _namingConvention make the this qualifier redundant.
  • get-only property with a readonly backing field is inherently thread-safe; more information here.
public class Neighborhood
{
    public Neighborhood(State left, State center, State right)
    {
        _left = left;
        _center = center;
        _right = right;
    }

    private readonly State _left;
    public State Left { get { return _left; } }

    private readonly State _center;
    public State Center { get { return _center; } }

    private readonly State _right;
    public State Right { get { return _right; } }
}
Community
  • 1
  • 1
Mathieu Guindon
  • 65,145
  • 8
  • 95
  • 208
  • I guess my class wasn't correct (immutable) after all. I prioritize clarity and correctness above concision, so this is *excellent*. – kdbanman Jul 09 '15 at 05:58
  • Just recognized something - should `Neighborhood` be [sealed](https://msdn.microsoft.com/en-us/library/88c54tsw) as well? Or are read only field safe for subclasses too? – kdbanman Jul 09 '15 at 06:05
  • Not sure I see how, but if [Jon Skeet says subclassing can break immutability](http://stackoverflow.com/a/268287/1188513), I believe him. Rule of thumb though, is to never seal a class, unless you have very good reasons to do so. – Mathieu Guindon Jul 09 '15 at 06:09
  • 1
    Ok. One more thing - why not shorten each data field to `public readonly State Left;`, and do `Left = left; ` in the constructor? Not sure if that's clear.. – kdbanman Jul 09 '15 at 06:11
  • You sure that would compile? – Mathieu Guindon Jul 09 '15 at 06:15
  • @kdbanman [don't expose public fields](http://stackoverflow.com/a/480637/1188513) – Mathieu Guindon Jul 09 '15 at 06:33
  • That rule is for the sake of maintainability. I could change a public field to a public property without breaking any dependent code, could I not? – kdbanman Jul 09 '15 at 06:38
  • @kdbanman http://stackoverflow.com/a/480638/1188513. and the rule is also about OOP best practices. – Mathieu Guindon Jul 09 '15 at 06:39
  • Languages like Python and C# allow classes to have properties with getters and setters, but are accessed and mutated as normal public fields. EX: `myInstance.someField = 42;`. In Java, you're *definitely* setting a field. Bad. But in Python or C#, you could be using a setter under the hood. So if one can easily change from a public field to a public getter **without breaking dependent code**, then isn't the "no public fields" rule not so hard-and-fast? – kdbanman Jul 09 '15 at 06:46
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/82782/discussion-between-mats-mug-and-kdbanman). – Mathieu Guindon Jul 09 '15 at 06:47
  • Woop, you're right. I just found [relevant information](http://stackoverflow.com/questions/653536/difference-between-property-and-field-in-c-sharp-3-0). Such a switch messes with serialization, among other things. – kdbanman Jul 09 '15 at 06:50