0

I've just spent the best part of 2 days trying to track down a bug, it turns out I was accidentally mutating the values that were provided as input to a function.

    IEnumerable<DataLog>
    FilterIIR(
        IEnumerable<DataLog> buffer
    ) {
        double notFilter = 1.0 - FilterStrength;

        var filteredVal = buffer.FirstOrDefault()?.oilTemp ?? 0.0;
        foreach (var item in buffer)
        {
            filteredVal = (item.oilTemp * notFilter) + (filteredVal * FilterStrength);

            /* Mistake here!
            item.oilTemp = filteredValue;
            yield return item;
            */

            // Correct version!
            yield return new DataLog()
            {
                oilTemp = (float)filteredVal,
                ambTemp = item.ambTemp,
                oilCond = item.oilCond,
                logTime = item.logTime
            };
        }
    }

My programming language of preference is usually C# or C++ depending on what I think suits the requirements better (this is part of a larger program that suits C# better)...

Now in C++ I would have been able to guard against such a mistake by accepting constant iterators which prevent you from being able to modify the values as you retrieve them (though I might need to build a new container for the return value). I've done a little searching and can't find any simple way to do this in C#, does anyone know different?

I was thinking I could make an IReadOnlyEnumerable<T> class which takes an IEnumerable as a constructor, but then I realized that unless it makes a copy of the values as you retrieve them it won't actually have any effect, because the underlying value can still be modified.

Is there any way I might be able to protect against such errors in future? Some wrapper class, or even if it's a small code snippet at the top of each function I want to protect, anything would be fine really.

The only sort of reasonable approach I can think of at the moment that'll work is to define a ReadOnly version of every class I need, then have a non-readonly version that inherits and overloads the properties and adds functions to provide a mutable version of the same class.

Salah Akbari
  • 39,330
  • 10
  • 79
  • 109
TheBeardedQuack
  • 449
  • 4
  • 15
  • Have you considered just making `DataLog` an immutable class. – juharr Apr 02 '20 at 12:08
  • Thanks for the formatting check @Salah, though `realised` is not a mistake XD – TheBeardedQuack Apr 02 '20 at 12:08
  • @juharr I mentioned that in my last paragraph, unless that's not quite what you mean? – TheBeardedQuack Apr 02 '20 at 12:09
  • I'm saying embrace immutable and don't bother with a mutable version of the classes. – juharr Apr 02 '20 at 12:10
  • @TheBeardedQuack You're Welcome. You're right, it is not a mistake, but the spell-checker is suggesting to use "realized" instead, that's why I change it :) – Salah Akbari Apr 02 '20 at 12:13
  • 1
    @SalahAkbari FYI, they are different potential spelling, British-English (en-GB) with 's' versus American-English (en-US) with 'z' – Pac0 Apr 02 '20 at 12:22
  • did you check https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.list-1.asreadonly?view=netframework-4.8 ? so you can have wrapper for the DataLog and it returns the ReadonlyCollection to be used by other methods, and because its wrapped inside your class no one can modify it – Hany Habib Apr 02 '20 at 12:26
  • @juharr, having mutable data is often very useful. Though in this particular class there's only 4 values in that class so it wouldn't too critical having to return new instances when I need to modify a value. I have taken this approach before but as the data structure grew to fit more requirements it became unwieldy to have to specify every value in order to just change 1. – TheBeardedQuack Apr 02 '20 at 14:18
  • @Hany, I mentioned something similar in my last paragraph, but that would involve writing 2 classes for every object where I need this kind of behaviour. Sweeper's answer of using an interface could provide a very similar effect by have a RO interface, then a RW interface that implements the RO version, then a single class for the implementation. I can have my functions accept the RO or the RW interface depending on what they do. But it would be nice to have a built in mechanism to guarantee immutability. – TheBeardedQuack Apr 03 '20 at 07:25

1 Answers1

1

The problem is here isn't really about the IEnumerable. IEnumerables are actually immutable. You can't add or remove things from them. What's mutable is your DataLog class.

Because DataLog is a reference type, item holds a reference to the original object, instead of a copy of the object. This, plus the fact that DataLog is mutable, allows you to mutate the parameters passed in.

So on a high level, you can either:

  • make a copy of DataLog, or;
  • make DataLog immutable

or both...

What you are doing now is "making a copy of DataLog". Another way of doing this is changing DataLog from a class to a struct. This way, you'll always create a copy of it when passing it to methods (unless you mark the parameter with ref). So be careful when using this method because it might silently break existing methods that assume a pass-by-reference semantic.

You can also make DataLog immutable. This means removing all the setters. Optionally, you can add methods named WithXXX that returns a copy of the object with only one property different. If you chose to do this, your FilterIIR would look like:

yield return item.WithOilTemp(filteredVal);

The only sort of reasonable approach I can think of at the moment that'll work is to define a ReadOnly version of every class I need, then have a non-readonly version that inherits and overloads the properties and adds functions to provide a mutable version of the same class.

You don't actually need to do this. Notice how List<T> implements IReadOnlyList<T>, even though List<T> is clearly mutable. You could write an interface called IReadOnlyDataLog. This interface would only have the getters of DataLog. Then, have FilterIIR accept a IEnumerable<IReadOnlyDataLog> and DataLog implement IReadOnlyDataLog. This way, you will not accidentally mutate the DataLog objects in FilterIIR.

Sweeper
  • 213,210
  • 22
  • 193
  • 313
  • Hi @Sweeper, I hadn't thought of using the interface approach, but this is effectively what I meant with making a readonly base class and then inheriting. The interface approach is much neater because then I only have 1 location that contains the implementation. I know that `IEnumerable` is immutable already and perhaps I could've done a better job at explaining myself. When I have a `const` container in C++ it also forces immutability on the items that it contains by having all the accessors return constant references. – TheBeardedQuack Apr 02 '20 at 13:21
  • While the `WithX()` approach is interesting, I don't think that would lead to a particularly pretty code-base. +1 for the interface suggestion though as I somehow completely missed that. – TheBeardedQuack Apr 02 '20 at 13:25
  • @TheBeardedQuack Well, C# isn't C++, so you can't translate everything across directly. Translating an English text _word-by-word_ into German would produce weird-sounding German, wouldn't it? :) You'll just have to make do with what the language _has_ to offer. IMO, the `IReadOnlyDataLog` approach is the best _overall_ too. – Sweeper Apr 02 '20 at 13:28
  • Yes I understand that, and C# definitely has certain ways of doing things that I would love to be available in C++. I was attempting to use C++ to explain my point, and then ask if there's any similar mechanism. As said I do think the interface approach is a decent idea. I do often use interfaces to help define my custom types but I've not usually had much need for defining my own so I'm probably not as accustomed to thinking in this manor as I should be. I may be using this approach much more in future... I'm happy to mark as answer if no better suggestions (for this scenario) come along. – TheBeardedQuack Apr 02 '20 at 14:05