Public member exposes a concrete collection type instead of an interface

ID

vbnet.maintainability.do_not_expose_generic_lists

Severity

low

Remediation Complexity

medium

Remediation Risk

medium

Remediation Effort

medium

Resource

Api Design

Language

VB.NET

Description

Reports a Public member whose signature names a concrete collection type rather than a collection interface: the property or field type, the return type of a function, or the type of a parameter is List(Of T), Dictionary(Of TKey, TValue), SortedList, Queue(Of T), Stack(Of T), ArrayList or Hashtable, with or without a System.Collections prefix and with or without an array suffix. Members declared on an interface are examined as well. Only the declared type name is inspected, so an interface type such as IList(Of T), IReadOnlyDictionary(Of TKey, TValue) or IEnumerable(Of T) is accepted, as is a type designed to be exposed such as ReadOnlyCollection(Of T) or a Collection(Of T) subclass. Non-public class members are not examined.

Rationale

A concrete collection in a public signature makes an implementation choice part of the contract. Once a property is typed List(Of OrderLine), callers may legitimately depend on anything List offers - Capacity, Sort, AddRange, indexer assignment - so the type can never be changed to a different collection, wrapped for validation, made lazy, or replaced by a result that streams instead of materializing, without a breaking change. On a return value it also hands callers a mutable handle to the object’s own state: book.Lines.Clear() reaches past every invariant the class enforces, and there is no place to intercept the mutation because the caller never calls a member of the class. Concrete collection types have no variance either, so a method taking List(Of OrderLine) rejects an array, a HashSet, a LINQ query result, or any other sequence the caller already has, forcing a defensive ToList() at every call site - an extra copy whose only purpose is to satisfy the signature. ArrayList and Hashtable carry the further problem of being untyped, so every read needs a cast and a type error surfaces at run time rather than at compile time.

The following code illustrates the pattern detected by this rule:

Public Class OrderBook

    ' FLAGGED: Public member exposes a concrete collection type instead of an interface
    Public Property Lines As List(Of OrderLine)

    ' FLAGGED: Public member exposes a concrete collection type instead of an interface
    Public Totals As Dictionary(Of String, Decimal)

    ' FLAGGED: Public member exposes a concrete collection type instead of an interface
    Public Shared Cache As Hashtable = New Hashtable()

    ' FLAGGED: Public member exposes a concrete collection type instead of an interface
    Public Function GetPendingLines() As List(Of OrderLine)
        Return New List(Of OrderLine)()
    End Function

Remediation

Type the member with the least capable interface that expresses what callers actually need. For a sequence they only read, use IEnumerable(Of T); for one they index, use IList(Of T) or - when they must not modify it - IReadOnlyList(Of T); for a keyed lookup use IDictionary(Of TKey, TValue) or IReadOnlyDictionary(Of TKey, TValue). On a parameter this is a pure widening change and callers keep compiling. On a property or return value returning an interface is not by itself protection - hand back New ReadOnlyCollection(Of T)(items) or a copy if the caller must not mutate the object’s state, and expose Add/Remove members on the declaring type instead of a settable collection. Replace ArrayList and Hashtable outright with their generic counterparts so element types are checked at compile time.

Configuration

This detector does not need any configuration.

References