patterncsharpMinor
Class simulating a circuit pin
Viewed 0 times
classpinsimulatingcircuit
Problem
This class is the first of an open source project I just (a couple of days ago) started. The projects is meant to simulate circuits (logic gates and stuff) and this class is meant to manage pins. I'm developing everything following the Observer design pattern.
Could you take a look at the code and tell me anything you see that is wrong or could be better? I mean anything: readability issues, naming, comments, indentation, you name it.
```
///
/// A pin used in the various logical gates.
///
public class Pin : Observable, IComparable, IComparable
{
#region variables
private PinValue _value;
private string _code;
private string _label;
#endregion
#region properties
///
/// Get the ID of the Pin.
///
public Guid Id
{
get;
private set;
}
///
/// Get or Set the value of the Pin.
///
public PinValue Value
{
get
{
return _value;
}
set
{
_value = value;
Notify(this);
}
}
///
/// Get or Set the code of the Pin.
///
public string Code
{
get
{
return _code;
}
set
{
_code = value;
Notify(this);
}
}
///
/// Get or Set the label of the Pin.
///
public string Label
{
get
{
return _label;
}
set
{
_label = value;
Notify(this);
}
}
#endregion
#region constructors
///
/// The default constructor of a Pin.
///
public Pin()
{
Id = Guid.NewGuid();
Value = PinValue.Unknown;
Code = Id.ToString();
Label = null;
}
///
/// A constructor of a Pin.
///
///
/// The initial value of the new Pin.
///
public Pin(PinValue value)
: this()
{
Value = value;
}
///
/// A const
Could you take a look at the code and tell me anything you see that is wrong or could be better? I mean anything: readability issues, naming, comments, indentation, you name it.
```
///
/// A pin used in the various logical gates.
///
public class Pin : Observable, IComparable, IComparable
{
#region variables
private PinValue _value;
private string _code;
private string _label;
#endregion
#region properties
///
/// Get the ID of the Pin.
///
public Guid Id
{
get;
private set;
}
///
/// Get or Set the value of the Pin.
///
public PinValue Value
{
get
{
return _value;
}
set
{
_value = value;
Notify(this);
}
}
///
/// Get or Set the code of the Pin.
///
public string Code
{
get
{
return _code;
}
set
{
_code = value;
Notify(this);
}
}
///
/// Get or Set the label of the Pin.
///
public string Label
{
get
{
return _label;
}
set
{
_label = value;
Notify(this);
}
}
#endregion
#region constructors
///
/// The default constructor of a Pin.
///
public Pin()
{
Id = Guid.NewGuid();
Value = PinValue.Unknown;
Code = Id.ToString();
Label = null;
}
///
/// A constructor of a Pin.
///
///
/// The initial value of the new Pin.
///
public Pin(PinValue value)
: this()
{
Value = value;
}
///
/// A const
Solution
Some bigger picture thoughts:
Honestly it sounds to me like you are conflating two distinct concepts here.
One is a "digital value" concept that supports all the various operations, and comparisons. That should probably be an immutable value type (struct), and would have helpful implicit conversions. Then you have a separate pin class that is observable, but not comparable or convertible, and which uses the digital value class as its value property. This separate pin class would have the GUID and label, etc.
Consider how the class is right now. You have comparison operators defined on the pins. The problem is that logically you are not comparing the pins. You are logically comparing the values on the pins. So it is better to have the pins store a custom value type with the comparison operators implemented on them. Similarly with the conversions to or from other data types. This is especially true with the conversions to a
Also as it is right now, The Equals method provides no value. Because the GUID is immutable and unique per instance, Equals is Equivalent to ReferenceEquals, which is the default implementation of Equals. Just drop Equals and GetHashCode.
So I would suggest that the Pin class would look more like:
As for the LogicValue class I've posted an implementation as a Gist (because it is a bit long, there are licensing considerations for posting it here).
So I've basically replaced the
The underlying idea is to separate out the logic about calculating values from a class that merely represents the pin of a component/gate.
Honestly it sounds to me like you are conflating two distinct concepts here.
One is a "digital value" concept that supports all the various operations, and comparisons. That should probably be an immutable value type (struct), and would have helpful implicit conversions. Then you have a separate pin class that is observable, but not comparable or convertible, and which uses the digital value class as its value property. This separate pin class would have the GUID and label, etc.
Consider how the class is right now. You have comparison operators defined on the pins. The problem is that logically you are not comparing the pins. You are logically comparing the values on the pins. So it is better to have the pins store a custom value type with the comparison operators implemented on them. Similarly with the conversions to or from other data types. This is especially true with the conversions to a
Pin, because Pins are parts of Gates (or other components) and we generally don't want to just create them out of thin ain, except as part of creating a gate itself. Also as it is right now, The Equals method provides no value. Because the GUID is immutable and unique per instance, Equals is Equivalent to ReferenceEquals, which is the default implementation of Equals. Just drop Equals and GetHashCode.
So I would suggest that the Pin class would look more like:
public class Pin : Observable
{
private LogicValue _value;
private string _code;
private string _label;
public Guid Id
{
get;
private set;
}
public PinValue Value
{
get
{
return _value;
}
set
{
_value = value;
Notify(this);
}
}
public string Code
{
get
{
return _code;
}
set
{
_code = value;
Notify(this);
}
}
public string Label
{
get
{
return _label;
}
set
{
_label = value;
Notify(this);
}
}
#endregion
#region constructors
public Pin()
{
Id = Guid.NewGuid();
Value = PinValue.Unknown;
Code = Id.ToString();
Label = null;
}
public Pin(string code)
: this()
{
Code = code;
}
public Pin(LogicValue value, string code)
: this(value)
{
Code = code;
}
public Pin(string code, string label)
: this(code)
{
Label = label;
}
public Pin(LogicValue value, string code, string label)
: this(value, code)
{
Label = label;
}
}As for the LogicValue class I've posted an implementation as a Gist (because it is a bit long, there are licensing considerations for posting it here).
So I've basically replaced the
PinValue enum with a more full featured class that encapsulates the calculations on logic Values. Then the pin becomes more an an observable set of attributes, at least for the moment. At some point I presume you will be adding a way to wire pins together, and perhaps at that point, you would add methods so that an input pin can observe the wire it is connected to, and update it's value when the wire's value changes. Or perhaps not.The underlying idea is to separate out the logic about calculating values from a class that merely represents the pin of a component/gate.
Code Snippets
public class Pin : Observable<Pin>
{
private LogicValue _value;
private string _code;
private string _label;
public Guid Id
{
get;
private set;
}
public PinValue Value
{
get
{
return _value;
}
set
{
_value = value;
Notify(this);
}
}
public string Code
{
get
{
return _code;
}
set
{
_code = value;
Notify(this);
}
}
public string Label
{
get
{
return _label;
}
set
{
_label = value;
Notify(this);
}
}
#endregion
#region constructors
public Pin()
{
Id = Guid.NewGuid();
Value = PinValue.Unknown;
Code = Id.ToString();
Label = null;
}
public Pin(string code)
: this()
{
Code = code;
}
public Pin(LogicValue value, string code)
: this(value)
{
Code = code;
}
public Pin(string code, string label)
: this(code)
{
Label = label;
}
public Pin(LogicValue value, string code, string label)
: this(value, code)
{
Label = label;
}
}Context
StackExchange Code Review Q#106030, answer score: 5
Revisions (0)
No revisions yet.