6

A third-party generated proxy that we use exposed the BLOB data type as a byte[], and we then expose this value through code generation as follows:

public byte[] FileRawData
{
    get { return internalDataRow.FileRawData; }
    set { this.internalDataRow.FileRawData = value; }
}

This property is then used througout our application (potentially in different assemblies). According to the FxCop rules, properties shouldn't be exposing arrays. So, what's the better (or best) approach here? What do others do in this situation?

  1. Switch to a method for these types.
  2. Change to a collection (ie ICollection<T> or IList<T> implementation)
  3. Turn off this FxCop rule.

Option 3 is always possible, but if we should be doing things differently, then I'd prefer that.

TMB
  • 4,683
  • 4
  • 25
  • 44
Daniel Becroft
  • 716
  • 3
  • 19
  • 2
    No, fxcop is actually warning you about something else. Returning a *deep copy* of the array is very inefficient. It is not smart enough to see what you are really doing, returning a *reference*. Far worse, any client code can change the array without you knowing about it. You'd better make that class *internal*. – Hans Passant Sep 11 '11 at 00:59
  • Is there any reason the code accessing the property should *not* be modifying the array? – Gabe Sep 11 '11 at 01:01
  • Hans: Making it internal is not an option, since this occurs in a common assembly, which other parts of our system access. Am I right in thinking that I should be returning a `ReadOnlyCollection` or similar? The implementation for `internalDataRow` just passes back a reference to its array as well, so wouldn't returning anything other than that be inefficient as well? – Daniel Becroft Sep 11 '11 at 01:52
  • @Daniel: Yes, a `ReadOnlyCollection` would be ideal if the clients of your API will not be modifying the array. – Gabe Sep 11 '11 at 04:08

1 Answers1

2

The usual problem in this sort of situation is immutability. When the byte[] gets returned the caller can change it, without go through your setter. Consider what happens if someone did

byte[] retVal = MyInstance.FileRawData;
retVal[1] = 0x00;

Probably not what you want at all, because the value has changed inside MyInstance, which could cause problems. So in order to stop that you clone the array, but that can be potentially long running, and properties shouldn't be used for long running operations. Best way to solve it is switch to methods for sets and gets, unless the array is always going to be tiny. Of course when you start writing GetFileRawData() as a method name FXCop will prompt you that it should be a property, you can't win grin In that case just disable it in code; for that one method.

blowdart
  • 55,577
  • 12
  • 114
  • 149
  • But that same problem would arise if I expose the array as a `List`, or `Collection`, wouldn't it? A caller could do `myInstance.FileRawData.Delete(index)`? The only way around that would be to expose a `ReadOnlyCollection`. – Daniel Becroft Sep 11 '11 at 03:01
  • 1
    Well, in order make a list it's going to perform a copy. So if you're returning the list or collection which is created from your byte array, rather than just substitute the list for a byte internally you'd be good (but ReadOnly would make more sense) – blowdart Sep 11 '11 at 03:12