Logic gates simulation

Question

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<Pin>, 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<PinValue> InputValues { get; }

    public IEnumerable<PinValue> 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;
        }
        protected set
        {
            _input = value;

            if (_inputUnsubscriber != null)
            {
                _inputUnsubscriber.Dispose();
            }

            if (_input != null)
            {
                _inputUnsubscriber = _input.Subscribe(this);
            }

            _input.Notify(_input);
        }
    }

    public override IEnumerable<PinValue> InputValues
    {
        get
        {
            var result = new PinValue[0];

            if (Input != null)
            {
                result = new PinValue[] { Input.Value };
            }

            return result;
        }
    }
}

BinaryGate.cs

public abstract class BinaryGate : Gate
{
    // variables
    private Pin _input1;
    private Pin _input2;
    private IDisposable _input1Unsubscriber;
    private IDisposable _input2Unsubscriber;

    // properties
    public Pin Input1
    {
        get
        {
            return _input1;
        }
        protected set
        {
            _input1 = value;

            if (_input1Unsubscriber != null)
            {
                _input1Unsubscriber.Dispose();
            }

            if (_input1 != null)
            {
                _input1Unsubscriber = _input1.Subscribe(this);
            }

            _input1.Notify(_input1);
        }
    }

    public Pin Input2
    {
        get
        {
            return _input2;
        }
        protected set
        {
            _input2 = value;

            if (_input2Unsubscriber != null)
            {
                _input2Unsubscriber.Dispose();
            }

            if (_input2 != null)
            {
                _input2Unsubscriber = _input2.Subscribe(this);
            }

            _input2.Notify(_input2);
        }
    }

    public override IEnumerable<PinValue> InputValues
    {
        get
        {
            var result = new List<PinValue>();

            if (Input1 != null)
            {
                result.Add(Input1.Value);
            }

            if (Input2 != null)
            {
                result.Add(Input2.Value);
            }

            return result.ToArray();
        }
    }
}

AndGate.cs

public class AndGate : BinaryGate
{
    // constructors

    public AndGate(bool? input1, bool? input2)
    {
        Output = new Pin();
        Input1 = new Pin(input1);
        Input2 = new Pin(input2);
    }

    public AndGate()
        : this(null, null)
    { }

    // methods

    /// <summary>
    /// Not implemented. Don't use.
    /// </summary>
    public override void OnCompleted()
    {

    }

    public override void OnError(Exception error)
    {
        throw error;
    }

    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);
        }
    }
}

The AndGate.cs contains just an example of a logic gate, there are others of course, but the implementation is similar to the AndGate so I'm not putting them here.

Welcome back! I would recommend you add a little information at the top to describe the purpose of the code, so you get some more text on the summaries. Hopefully you get good reviews. :) — Der Kommissar
– Der Kommissar, Commented Oct 9, 2015 at 17:42
@Heslacher, I totally skipped that check. Thanks for pointing that out. — Gentian Kasa
– Gentian Kasa, Commented Oct 10, 2015 at 16:24

Der Kommissar · Accepted Answer · 2015-10-09 18:28:27Z

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.

Thanks for the excellent feedback @EBrown. Regarding C# 6.0 I'm not working with it yet, but plan to use some months down the road 'cause it has some useful things compared to the previous versions. — Gentian Kasa
– Gentian Kasa, Commented Oct 10, 2015 at 16:39

Heslacher · Accepted Answer · 2015-10-10 16:23:59Z

Gate

public IEnumerable<PinValue> OutputValues
{
    get
    {
        PinValue[] result = new PinValue[0];

        if (Output != null)
        {
            result = new PinValue[] { Output.Value };
        }

        return result;
    }
}

there is no need to first create an array and later overwrite it if Output != null. You can simply use yield return like so

public IEnumerable<PinValue> OutputValues
{
    get
    {
        if (Output != null) { yield return Output.Value; }
    }
}

This is much cleaner, isn't it ?

public abstract void OnCompleted();

public abstract void OnError(Exception error);

public abstract void OnNext(Pin value);

These methods are looking like some "event helpers" hence they shouldn't be public but protected. If they aren't these kind of event helpers, you should consider to change the name of them.

UnaryGate

This class has the same bug in the setter of the Input property like I mentioned in the comments and @EBrown mentioned in his answer.

For the public override IEnumerable<PinValue> InputValues you should use the same pattern as shown above.

BinaryGate

Regarding the setter like mentioned above, same bug, only 2 times here.

public override IEnumerable<PinValue> InputValues
{
    get
    {
        var result = new List<PinValue>();

        if (Input1 != null)
        {
            result.Add(Input1.Value);
        }

        if (Input2 != null)
        {
            result.Add(Input2.Value);
        }

        return result.ToArray();
    }
}

Again yield return only 2 times

public override IEnumerable<PinValue> InputValues
{
    get
    {

        if (Input1 != null)
        {
            yield return Input1.Value;
        }

        if (Input2 != null)
        {
            yield return Input2.Value;
        }
    }
}

there is no need to create a List<T> and the call ToArray() on it.

Regarding the three methods, they are methods of the interface System.IObserver<in T> that the class implements. — Gentian Kasa
– Gentian Kasa, Commented Oct 10, 2015 at 16:35
And the solution that uses yield return is surprisingly elegant. Thanks. — Gentian Kasa
– Gentian Kasa, Commented Oct 10, 2015 at 16:48

Stack Exchange Network

Logic gates simulation

2 Answers 2

Some C#6.0 Stuff

You must log in to answer this question.

Linked

Hot Network Questions

Logic gates simulation

2 Answers 2

Some C#6.0 Stuff

You must log in to answer this question.

Linked

Related

Hot Network Questions