0

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;
    }
}
heltonbiker
  • 26,657
  • 28
  • 137
  • 252
  • 1
    Is `ProcessNewSamples` called from multiple threads? If so, you should lock you access to the octet in the function. – Nico Sep 19 '16 at 19:49
  • @heinzbeinz Ok, I have found that the task where the code is running is called more than one time inadvertently. Well, this sure is a defect in the client code, and I'm gonna fix it. But regarding the `lock` strategy, could you post it as an answer? I'm not sure the best place to put the lock. Thank you very much! – heltonbiker Sep 19 '16 at 19:59

1 Answers1

2

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)
    }
}
Nico
  • 3,542
  • 24
  • 29