Skip to main content
Code Review

Return to Answer

replaced http://programmers.stackexchange.com/ with https://softwareengineering.stackexchange.com/
Source Link
#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.

Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467
#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.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /