Don't assign a field from many methods

Rule ND1906 Immutability

A field assigned from many methods is a symptom of bug-prone code (constructors assigning the field are not counted). Threshold: 4+ assigning methods for instance fields; 3+ for static fields, whose global random accesses are even worse. Such anarchically assigned fields make runtime state hard to anticipate: code hard to read, debug and maintain, with hard-to-solve corrupted-state bugs.

With multiple threads there are alarming chances the code is unmaintainable and bugged — the rule of thumb is immutable objects. Corrupted reference-type state can yield NullReferenceException; corrupted value-type state can yield silently wrong results.

How to fix

No straight advice. Sometimes it is simple: e.g. an indentation field assigned by many writers just needs IndentPlus()/IndentMinus() methods. Sometimes a complex algorithm must be rethought and rewritten: the field may become a local variable or closure, or rethinking the parent object's lifetime and role can make it immutable (constructor-assigned only).

Estimated Debt: 4 minutes plus 5 minutes per method assigning the instance field, or 10 minutes per method assigning the static field.

CQLinq source

This rule is written in CQLinq, C# LINQ over the NDepend code model. It ships with every new NDepend project: you can read it, adapt it, or use it as a starting point for your own custom rules.

// <Name>Don't assign a field from many methods</Name>
// <Id>ND1906:DontAssignAFieldFromManyMethods</Id>
            
warnif count > 0 
from f in JustMyCode.Fields where 
  !f.IsEnumValue &&
  !f.IsImmutable && 
  !f.IsInitOnly &&
  !f.IsGeneratedByCompiler &&
  !f.IsEventDelegateObject

let methodsAssigningMe = f.MethodsAssigningMe.Where(m => !m.IsConstructor)

// The thresholds 4 and 3 are arbitrary and it should avoid matching too many fields.
// Threshold is even lower for static fields because this reveals situations even more complex.
where methodsAssigningMe.Count() >= (!f.IsStatic ? 4 : 3)

select new { 
   f, 
   methodsAssigningMe, 
   f.MethodsReadingMeButNotAssigningMe, 
   f.MethodsUsingMe,
   staticOrInst = f.IsStatic ? "static" : "instance",
   Debt = (4+(f.IsStatic ? 10 : 5)).ToMinutes().ToDebt(),
   Severity = Severity.High
} 

//<Expl>
// The *{4} {0}* is assigned from the *{1}*.
// Do reduce the number of methods that assign it.
// By now there is no clear and predictable pattern 
// for how this state changes at runtime.
//</Expl>

//<Desc>
// A field assigned from many methods is a symptom of **bug-prone code**
// (constructors assigning the field are not counted). Threshold: 4+
// assigning methods for *instance fields*; 3+ for *static fields*, whose
// global random accesses are even worse. Such *anarchically assigned*
// fields make runtime state hard to anticipate: code hard to read, debug
// and maintain, with hard-to-solve corrupted-state bugs.
//
// With multiple threads there are **alarming chances** the code is
// unmaintainable and bugged — the rule of thumb is immutable objects.
// Corrupted reference-type state can yield *NullReferenceException*;
// corrupted value-type state can yield silently wrong results.
//</Desc>

//<HowToFix>
// No straight advice. Sometimes it is simple: e.g. an indentation field
// assigned by many writers just needs *IndentPlus()/IndentMinus()* methods.
// Sometimes a complex algorithm must be rethought and rewritten: the field
// may become a local variable or *closure*, or rethinking the parent
// object's lifetime and role can make it immutable (constructor-assigned
// only).
//
// Estimated Debt: 4 minutes plus 5 minutes per method assigning the
// instance field, or 10 minutes per method assigning the static field.
//</HowToFix>

Open in the interactive Rules Explorer · All default rules

Immutability: all 15 items