Search code examples
c#listcountcommand-query-separation

Property seems to be changing between method calls, but no code is supposed to be changing it


I have an Octet class that is suppose to "package" eight samples and then send them forward. It has methods to add a new sample, check if it is already full, and to extract a Frame datastructure built from the eight values from the Octet.

The Octet class throws two kinds of exceptions: "cannot extract because not yet full" and "cannot add sample because already full". For that, the client code should check if full before calling Add, and extract as soon as is full, as well as to reset it (quite a lame class contract, to be honest).

The problem is: I am getting the two kinds of errors, even though the client class - the only one using Octet - seems to be performing the checks correctly before the operations that throw, but even though the error conditions are being hit. To make matters worse, when I check the values when debugger breaks, they are correct, that is, the exceptions should not be throwing!

public class Client
{
    private Octet _octet = new Octet();

    void ProcessNewSamples(IEnumerable<int> newSamples)
    {
        foreach (int sample in newSamples)
        {
            if (!_octet.IsFull)
            {
                _octet.Add(sample);
            }

            if (_octet.IsFull)
            {
                var frame = _octet.ExtractFrame();
                this.SendElsewhere(frame);
                _octet.Reset();
            }

        }
    }
}


public class Octet
{
    const int ARRAY_SIZE = 8;
    int[] _samples = new int[ARRAY_SIZE];
    int _index = 0;

    public bool IsFull { get { return _index >= 8; } }

    public void Add(int sample)
    {
        if (IsFull)
        {
            throw new InvalidOperationException();
        }
        else
            _samples[_index++] = sample;
    }

    public Frame<int> ExtractFrame()
    {
        if (!IsFull)
            throw new InvalidOperationException();
        else
            return new Frame<int>(_samples);

    }

    public void Reset()
    {
        _samples = new int[ARRAY_SIZE];
        _index = 0;
    }
}

Solution

  • As mentioned in the comment, you should place a lock if your function is accessed in parallel.

    If SendElsewhere is fast, I simply would place the lock around the function:

    void ProcessNewSamples(IEnumerable<int> newSamples)
    {
        lock (this)
        {
            foreach (int sample in newSamples)
            {
                if (!_octet.IsFull)
                {
                    _octet.Add(sample);
                }
    
                if (_octet.IsFull)
                {
                    var frame = _octet.ExtractFrame();
                    this.SendElsewhere(frame);
                    _octet.Reset();
                }
            }
        }
    }
    

    Otherwise I would collect all frames and send them afterwards:

    void ProcessNewSamples(IEnumerable<int> newSamples)
    {
        var frames = new List<Frame>();
    
        lock (this)
        {
            foreach (int sample in newSamples)
            {
                if (!_octet.IsFull)
                {
                    _octet.Add(sample);
                }
    
                if (_octet.IsFull)
                {
                    var frame = _octet.ExtractFrame();
                    frames.Add(frame);
                    _octet.Reset();
                }
            }
        }
    
        foreach (var frame in frames)
        {
            this.SendElsewhere(frame)
        }
    }