Accessor Uses Expected Field

ID

csharp.accessor_uses_expected_field

Severity

high

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Code Smell

Language

CSharp

Tags

code_smell, copy-paste, field, suspicious-assignment

Description

Reports a property accessor that never touches the field named after the property, while it does read or write some other field of the same type. A field corresponds to a property when the two names match ignoring case and a leading or m, so Total corresponds to total, _total and m_total.

The whole accessor body is examined, not just a one-statement one: a guard, a conversion or a computation is exactly where a mistyped identifier survives review. Every field and this.field reference in the body is collected, and the accessor is reported only when the corresponding field is absent from that set. Fields of any access level count, since a protected or internal backing field is as easy to name wrongly as a private one.

Three situations are left alone: a property with no correspondingly named field, which is a computed or derived value; a property whose name corresponds to more than one field, such as when both y and _y are declared, because no convention can say which one was meant; and an accessor that reads no field of the type at all, which has nothing to weigh against the property name.

Rationale

This is the copy-paste defect of hand-written properties. A property is duplicated, the declaration is renamed, and the identifier inside the accessor is left behind. Everything still compiles and type-checks, because the wrong field has the right type.

Nothing points at the mistake at run time either. The property returns a real value from a real field, so one field silently becomes write-only and one property silently mirrors another. It surfaces much later as two values that refuse to disagree, or as a setter whose effect never appears anywhere — and the fix is a single identifier, unrelated to the time spent finding it.

public class Rectangle
{
    private int width;
    private int height;
    private string label;
    protected int depth;
    protected int span;

    public int Width => width;                  // OK

    public int Height => width;                 // FLAW, height is the matching field

    public int Depth
    {
        get { if (span < 0) return 0; return span; }  // FLAW, the guard does not excuse the wrong field
    }

    public string Label
    {
        get { return this.label; }              // OK, this-qualified is still the matching field
        set { label = value; }                  // OK
    }

    public int Area => width * height;           // OK, a computed property

    public int Diagonal => width;                // OK, no field named after Diagonal
}

Remediation

Read the accessor against the property name and use the field that belongs to it. Where the pattern is nothing but storage, an auto-implemented property removes the backing field from the source entirely and with it any chance of naming the wrong one:

public int Height { get; set; }

Where a backing field is needed — validation in the setter, lazy initialization, a field the constructor writes directly — keep the field but make the correspondence exact, so a mismatched identifier stands out while reading.

References