heltonbiker heltonbiker - 2 months ago 6
C# Question

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

Answer

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