No Only Private Ctors

ID

csharp.no_only_private_ctors

Severity

high

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Code Smell

Language

CSharp

Tags

api-design, code_smell, instantiation

Description

Reports classes whose every declared instance constructor is private, so the class can neither be instantiated nor derived from, while it still declares instance members that no code can reach. A constructor with no access modifier counts as private, since that is the C# default for class members.

Legitimate uses of a private constructor are excluded:

  • static and abstract classes, and partial classes whose other part may expose the factory;

  • classes with a primary constructor, with any non-private constructor, or with no declared instance constructor at all — whether none is declared or the only one is static;

  • classes that actually call their own private constructor — a new ThisClass(…​) anywhere the private constructor is reachable, which covers singletons, smart enums and nested builders; the C# 9 target-typed form (Instance = new();, Instance ⇒ new();) is recognised too;

  • classes deriving from a native handle base such as SafeHandle, whose instances the runtime materialises through P/Invoke marshalling;

  • classes whose members are all static — those are reported by csharp.static_class with the more useful advice.

Rationale

Nothing outside the class can call a private constructor, and no derived class can chain to it, so an instance can only be created by code that sits inside the class or its container. When no such code ever calls it, the class’s instance methods, properties and events are unreachable: they read as live API but can never run. This normally means one of two things went wrong — the constructor was supposed to be public (or protected for a base class), or the members were supposed to be static.

public class Report                         // FLAW - nothing can build a Report
{
    private Report() { }

    public string Title { get; set; }
    public void Render() { }
}

public class Session                        // OK - static factory hands out instances
{
    private Session() { }

    public static Session Open() { return new Session(); }
    public void Close() { }
}

public class Cache                          // OK - singleton idiom
{
    private static readonly Cache instance = new Cache();
    private Cache() { }

    public static Cache Instance => instance;
    public void Clear() { }
}

public class TargetTypedSingleton           // OK - C# 9 target-typed new()
{
    public static readonly TargetTypedSingleton Instance = new();
    private TargetTypedSingleton() { }

    public void Clear() { }
}

public class Suit                           // OK - smart enum builds its own instances
{
    public static readonly Suit Hearts = new Suit("hearts");
    private Suit(string name) { Name = name; }

    public string Name { get; }
}

internal sealed class NativeHandle : SafeHandle   // OK - the runtime creates it via P/Invoke
{
    private NativeHandle() : base(IntPtr.Zero, true) { }

    protected override bool ReleaseHandle() { return true; }
}

public static class Paths                   // OK - static class
{
    public static string Join(string a, string b) { return a + "/" + b; }
}

Remediation

Decide which of the two intents applies. If callers are meant to create instances, widen the constructor to public (or protected when the class is a base type). If the class is a pure helper, declare it static class and make its members static. If instances are meant to be handed out under control, add the static factory or singleton accessor that does so.

References