HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Logic gates simulation

Submitted by: @import:stackexchange-codereview··
0
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

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 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.