#region variables private PinValue _value; private string _code; private string _label; #endregion
They're not variables, they're fields. But the annoying part isn't the miswording - it's #region
. Get rid of them Get rid of them.
You have this Id
auto-property:
/// <summary> /// Get the ID of the Pin. /// </summary> public Guid Id { get; private set; }
And then this comment:
// Id is the only immutable value of this object
From the outside, it's true. But the Id
isn't immutable - it's an auto-property with a private setter, which means the class can change it whenever it feels like. C# 6.0 introduced actually immutable auto-properties; if you're not using C# 6.0, this is what an immutable property looks like:
private readonly int _id;
public int Id { get { return _id; } }
Notice how the backing field being right next to the associated public member defeats the purpose of #region
, and how it puts the related stuff right next to each other ;-)
I would take the LogicalValue
enum out of the enum. Maybe it's just me not liking nested types, but I find this uselessly verbose:
var pin = new Pin(PinValue.LogicalValue.Low);
This is starting to be very much about personal preference, but I find this:
public string Label { get { return _label; } set { _label = value; Notify(this); } }
Looks better like this:
public string Label
{
get { return _label; }
set
{
_label = value;
Notify(this);
}
}
The getter is straightforward; vertically extending only the setter puts some emphasis on it and its Notify
behavior.
#region variables private PinValue _value; private string _code; private string _label; #endregion
They're not variables, they're fields. But the annoying part isn't the miswording - it's #region
. Get rid of them.
You have this Id
auto-property:
/// <summary> /// Get the ID of the Pin. /// </summary> public Guid Id { get; private set; }
And then this comment:
// Id is the only immutable value of this object
From the outside, it's true. But the Id
isn't immutable - it's an auto-property with a private setter, which means the class can change it whenever it feels like. C# 6.0 introduced actually immutable auto-properties; if you're not using C# 6.0, this is what an immutable property looks like:
private readonly int _id;
public int Id { get { return _id; } }
Notice how the backing field being right next to the associated public member defeats the purpose of #region
, and how it puts the related stuff right next to each other ;-)
I would take the LogicalValue
enum out of the enum. Maybe it's just me not liking nested types, but I find this uselessly verbose:
var pin = new Pin(PinValue.LogicalValue.Low);
This is starting to be very much about personal preference, but I find this:
public string Label { get { return _label; } set { _label = value; Notify(this); } }
Looks better like this:
public string Label
{
get { return _label; }
set
{
_label = value;
Notify(this);
}
}
The getter is straightforward; vertically extending only the setter puts some emphasis on it and its Notify
behavior.
#region variables private PinValue _value; private string _code; private string _label; #endregion
They're not variables, they're fields. But the annoying part isn't the miswording - it's #region
. Get rid of them.
You have this Id
auto-property:
/// <summary> /// Get the ID of the Pin. /// </summary> public Guid Id { get; private set; }
And then this comment:
// Id is the only immutable value of this object
From the outside, it's true. But the Id
isn't immutable - it's an auto-property with a private setter, which means the class can change it whenever it feels like. C# 6.0 introduced actually immutable auto-properties; if you're not using C# 6.0, this is what an immutable property looks like:
private readonly int _id;
public int Id { get { return _id; } }
Notice how the backing field being right next to the associated public member defeats the purpose of #region
, and how it puts the related stuff right next to each other ;-)
I would take the LogicalValue
enum out of the enum. Maybe it's just me not liking nested types, but I find this uselessly verbose:
var pin = new Pin(PinValue.LogicalValue.Low);
This is starting to be very much about personal preference, but I find this:
public string Label { get { return _label; } set { _label = value; Notify(this); } }
Looks better like this:
public string Label
{
get { return _label; }
set
{
_label = value;
Notify(this);
}
}
The getter is straightforward; vertically extending only the setter puts some emphasis on it and its Notify
behavior.
#region variables private PinValue _value; private string _code; private string _label; #endregion
They're not variables, they're fields. But the annoying part isn't the miswording - it's #region
. Get rid of them.
You have this Id
auto-property:
/// <summary> /// Get the ID of the Pin. /// </summary> public Guid Id { get; private set; }
And then this comment:
// Id is the only immutable value of this object
From the outside, it's true. But the Id
isn't immutable - it's an auto-property with a private setter, which means the class can change it whenever it feels like. C# 6.0 introduced actually immutable auto-properties; if you're not using C# 6.0, this is what an immutable property looks like:
private readonly int _id;
public int Id { get { return _id; } }
Notice how the backing field being right next to the associated public member defeats the purpose of #region
, and how it puts the related stuff right next to each other ;-)
I would take the LogicalValue
enum out of the enum. Maybe it's just me not liking nested types, but I find this uselessly verbose:
var pin = new Pin(PinValue.LogicalValue.Low);
This is starting to be very much about personal preference, but I find this:
public string Label { get { return _label; } set { _label = value; Notify(this); } }
Looks better like this:
public string Label
{
get { return _label; }
set
{
_label = value;
Notify(this);
}
}
The getter is straightforward; vertically extending only the setter puts some emphasis on it and its Notify
behavior.