Externally visible collection property is writable
ID |
vbnet.maintainability.collection_properties_should_be_read_only |
Severity |
low |
Remediation Complexity |
medium |
Remediation Risk |
medium |
Remediation Effort |
low |
Resource |
Api Design |
Language |
VB.NET |
Description
Reports a Public, Protected or Friend property whose type is a mutable collection - a
generic List(Of T), Dictionary(Of K, V), HashSet(Of T), ObservableCollection(Of T),
Queue/Stack, one of the IList/ICollection/IDictionary/ISet interfaces, or a
non-generic ArrayList, Hashtable, SortedList or NameValueCollection - and that is not
declared ReadOnly, so callers can assign a different collection instance to it. The form that
initialises the collection in the declaration, with As New List(Of T)() or with = New
List(Of T)(), is reported as well. Properties declared ReadOnly, properties that are not
externally visible, and fields of the same collection types are not reported.
Rationale
A writable collection property exposes two different powers, and only one of them is ever
wanted. Callers need to add and remove items, which the collection object itself already allows
through a read-only property. The setter adds the power to swap the whole collection, and
nothing good is reachable through it. Anything the type derived from the old instance is
invalidated silently: an event handler attached to an ObservableCollection, a comparer chosen
for a HashSet, a Dictionary created with a case-insensitive comparer - all replaced by
whatever the caller happened to construct, so lookups start failing on casing or on ordering
with no change in the type’s own code. A caller can also assign Nothing, and from then on
every method of the type has to defend against a null collection or throw. The worst case is
aliasing: the caller keeps its reference and continues to mutate the collection the object is
now using, so two components share mutable state that neither’s code suggests is shared. Making
the property ReadOnly and initialising the collection once at construction removes all of
these, and costs callers nothing they legitimately needed.
The following code illustrates the pattern detected by this rule:
Private _lines As New List(Of OrderLine)()
' FLAGGED: Externally visible collection property is writable
Public Property Lines As List(Of OrderLine)
' FLAGGED: Externally visible collection property is writable
Public Property Totals As Dictionary(Of String, Decimal)
' FLAGGED: Externally visible collection property is writable
Friend Property LegacyCodes As ArrayList
' FLAGGED: Externally visible collection property is writable
Protected Overridable Property Watchers As ObservableCollection(Of Watcher)
Get
Return _watchers
End Get
Set(value As ObservableCollection(Of Watcher))
_watchers = value
End Set
End Property
' FLAGGED: Externally visible collection property is writable
Public Property Discounts As New List(Of Discount)()
' FLAGGED: Externally visible collection property is writable
Public Property Adjustments As Dictionary(Of String, Decimal) = New Dictionary(Of String, Decimal)()Remediation
Declare the property ReadOnly and initialise the backing collection at construction, letting
callers use Add, Remove and Clear on the instance the type owns. Where callers really do
need to supply a whole set of items, offer a method such as ReplaceLines(items) that copies
into the existing collection, so the type keeps its instance, its comparer and its event
subscriptions. Expose IReadOnlyList(Of T) or ReadOnlyCollection(Of T) when the contents are
not meant to be modified from outside at all.
' Before: a caller can swap the collection, or assign Nothing
Public Property Lines As List(Of OrderLine)
' After
Private ReadOnly _lines As New List(Of OrderLine)()
Public ReadOnly Property Lines As IList(Of OrderLine)
Get
Return _lines
End Get
End Property
Public Sub ReplaceLines(items As IEnumerable(Of OrderLine))
_lines.Clear()
_lines.AddRange(items)
End Sub