patterncsharpMinor
Logic gates simulation
Viewed 0 times
simulationgateslogic
Problem
Following this post and this other post here we are with another review request. The code that follows contains the definition of logic gates (binary gates like the AND, OR gate, and unary gates like NOT). They are a part of an open-source application I have started to build. I'm following the Observer pattern to build the application.
As always, feel free to be merciless with the review, throw me anything you can find:
Gate.cs
UnaryGate.cs
```
public abstract class UnaryGate : Gate
{
// variables
private Pin _input;
private IDisposable _inputUnsubscriber;
// properties
public Pin Input
{
get
{
return _input;
As always, feel free to be merciless with the review, throw me anything you can find:
Gate.cs
public abstract class Gate : IObserver, ILogicComponent
{
// fields
private readonly Guid _id = Guid.NewGuid();
private Pin _output;
private IDisposable _outputUnsubscriber;
// properties
public Guid Id
{
get
{
return _id;
}
}
public string Code { get; set; }
public string Label { get; set; }
public Pin Output
{
get
{
return _output;
}
protected set
{
_output = value;
if (_outputUnsubscriber != null)
{
_outputUnsubscriber.Dispose();
}
if (_output != null)
{
_outputUnsubscriber = _output.Subscribe(this);
}
_output.Notify(_output);
}
}
public abstract IEnumerable InputValues { get; }
public IEnumerable OutputValues
{
get
{
PinValue[] result = new PinValue[0];
if (Output != null)
{
result = new PinValue[] { Output.Value };
}
return result;
}
}
// methods
public abstract void OnCompleted();
public abstract void OnError(Exception error);
public abstract void OnNext(Pin value);
}UnaryGate.cs
```
public abstract class UnaryGate : Gate
{
// variables
private Pin _input;
private IDisposable _inputUnsubscriber;
// properties
public Pin Input
{
get
{
return _input;
Solution
One of the first changes I would make is to change how the
Assuming you already have an equality check for
This means you won't change
This also means the following method will simplify:
To:
This is advantageous as you already did the comparisons twice, and it's clear that you don't want
Just as well, I think this is a bug:
What happens when
Some C#6.0 Stuff
Whether or not you have access to it, I find it helpful to include these things so that other developers can be pointed to a good example of what C#6.0 can do for you/them.
For methods that only have one statement that is a
Of course, you no longer need to for C#6.0 with read-only properties.
This is the exact same as specifying a read-only field that backs the property, and initializing that field. The difference is that you no longer need to explicitly initialize the field (in the constructor). This works exactly the same as a read-only field backed
You can also use the null method-invocation syntax.
Can instead be written as:
Which silently does the null-check.
You can also use it in statements, but only where
(Unless you want to assign
It would also help you out with:
Which can safely run as:
Which will either do nothing, or setup the notifier.
set mutator on Output works.Assuming you already have an equality check for
Pin (and if you don't, I would make one) you can add the following to the beginning of the mutator on Gate.OutputValue:if (_output == value)
{
return;
}This means you won't change
_output if there's nothing to change, and you won't notify that _output has changed (because nothing changed).This also means the following method will simplify:
public override void OnNext(Pin value)
{
if (Output != null
&& Input1 != null
&& Input2 != null
&& Output.Value != (Input1.Value && Input2.Value))
{
Output.Value = (Input1.Value && Input2.Value);
}
}To:
public override void OnNext(Pin value)
{
if (Output != null
&& Input1 != null
&& Input2 != null)
{
Output.Value = (Input1.Value && Input2.Value);
}
}This is advantageous as you already did the comparisons twice, and it's clear that you don't want
OutputValue.Set to accidentally trigger things. Only if it's value has actually changed do you want it to update. (At least, from what I can see.)Just as well, I think this is a bug:
if (_output != null)
{
_outputUnsubscriber = _output.Subscribe(this);
}
_output.Notify(_output);What happens when
_output is null?Some C#6.0 Stuff
Whether or not you have access to it, I find it helpful to include these things so that other developers can be pointed to a good example of what C#6.0 can do for you/them.
For methods that only have one statement that is a
return, and for get-only properties, you can use expression-bodied members to handle them.public Guid Id => _id;Of course, you no longer need to for C#6.0 with read-only properties.
public Guid Id { get; } = Guid.NewGuid();This is the exact same as specifying a read-only field that backs the property, and initializing that field. The difference is that you no longer need to explicitly initialize the field (in the constructor). This works exactly the same as a read-only field backed
get-only property. (I know, that's a mouthful.)You can also use the null method-invocation syntax.
if (_outputUnsubscriber != null)
{
_outputUnsubscriber.Dispose();
}Can instead be written as:
_outputUnsubscriber?.Dispose();Which silently does the null-check.
You can also use it in statements, but only where
null or Type are valid values. So for example, the following would be invalid:_input1Unsubscriber = _input1?.Subscribe(this);(Unless you want to assign
null to the _input1Unsubscriber if _input1 is null.)It would also help you out with:
_output.Notify(_output);Which can safely run as:
_output?.Notify(_output);Which will either do nothing, or setup the notifier.
Code Snippets
if (_output == value)
{
return;
}public override void OnNext(Pin value)
{
if (Output != null
&& Input1 != null
&& Input2 != null
&& Output.Value != (Input1.Value && Input2.Value))
{
Output.Value = (Input1.Value && Input2.Value);
}
}public override void OnNext(Pin value)
{
if (Output != null
&& Input1 != null
&& Input2 != null)
{
Output.Value = (Input1.Value && Input2.Value);
}
}if (_output != null)
{
_outputUnsubscriber = _output.Subscribe(this);
}
_output.Notify(_output);public Guid Id => _id;Context
StackExchange Code Review Q#107081, answer score: 7
Revisions (0)
No revisions yet.