How to separate public and "mostly private" code in C#? (Friend classes, PIMPL pattern, etc.)

Question

Reminder: If you have tips, please remember to put the reason objectively, such as "having two distinct SetInt() functions in the same file violates reader expectations that they'll be overloads, and stymies the ability to find the right function with ctrl+F."

Problem:

Occasionally, low level/dangerous APIs are needed. They should be hidden, but they can't be private. For example, functions to manipulate an additional (not loaded) save data, rename/delete the underlying save data config files on disk, or disallow any future saves from being written to disk. How do I keep the API clean, so normal classes don't see these ancillary methods?

In C++, these ancillary functions would be private, and the classes that needed access would be friend classes. In C#, my first thought was to use this:

Pseudo-Facade pattern, hiding ancillary methods:

// The Save.Foo() functions are used all the time. Concise/simple API needed. 
public static class Save
{
    public static void SetInt(string key, int value)
    {
        Save_Implementation.SetInt(currentSaveData, key, value);
    }
}

public static class Save_Implementation // ancillary or dangerous APIs
{
    public static void SetInt(object data, string key, int value) { /* ... */ }
    public static void StopAllSaves() { /* ... */ }
    public static string GetCurrentFilename() { /* ... */ }
}

This works, but I should related the classes to better clarify that they're part of the same thing. Subclass? Can't, it's static. Namespace? Can't, we need to type these function names with brutal frequency. Nested class? Yes, please.

Nested class:

public static class Save
{
    public static void SetInt(string key, int value)
    {
        Impl.SetInt(currentSaveData key, value);
    }

    public static class Impl // ancillary or dangerous APIs
    {
        public static void SetInt(object data, string key, int value) { /* ... */ }
        public static void StopAllSaves() { /* ... */ }
        public static string GetCurrentFilename() { /* ... */ }
    }
}

That's better, but messier than I'd prefer, since there are duplicated methods in the same file. (I.e., SetInt() calls Impl.SetInt(currentSaveData).) Next I tried splitting it up:

Partial classes, with ancillary stuff in a different file:

Save.cs:

public static partial class Save
{
    public static void SetInt(string key, int value)
    {
        Impl.SetInt(currentSaveData, key, value);
    }

    public static partial class Impl // ancillary or dangerous APIs
    {
    }
}

SaveImplementation.cs:

public static partial class Save
{
    public static partial class Impl // ancillary or dangerous APIs
    {
        public static void SetInt(object data, string key, int value) { /* ... */ }
        public static void StopAllSaves() { /* ... */ }
        public static string GetCurrentFilename() { /* ... */ }
    }
}

This works, but the IDE isn't smart enough to open the right file when I jump to the definition of Save or Save.Impl. Is there a perfect way to organize this, or will it always be a trade-off?

Edit: Save is a drop-in replacement for the game engine's default save functionality, so it's ideal for it to clone the existing API (and be static).

Answer 1

Partial classes are mostly intended to separate generated code from non-generated code, so that changes can occur in the non-generated code that don't affect the generated code. Is that the case here?

Internal classes are meant to gather together stuff that belongs together conceptually, but will never be used outside the class. They are fairly rare beasts. static ones are vanishingly rare. Nothing you've shown here indicates a need for them.

Impl classes are meant to service Interfaces, which have their own specific purpose (i.e. the ability to swap implementations). You don't seem to have that requirement.

At the end of the day, a single class (acting as an adapter) for each "dangerous" API may be all the encapsulation you will ever need, unless your conversion code is complex enough to warrant the additional architecture.

Answer 2

As both the normal features and "here be dragons" advanced features need to be publicly accessible, but the latter shouldn't normally be used, then separate them into two assemblies: a "normal usage" one and an "advanced features" one. That way, the advanced features cannot be accidentally used: a developer must explicitly add that assembly as a reference to access those features.

Answer 3

Answering my own question--there is a solution that's embarrassingly simple: while the API needs to be static*, the class itself does not need to be. With that little change, the compiler will allow inheritance. The highly visible methods can go in the parent class, and adapters can inherit from the parent class to get more intimate access. The "don't call this unless you know what you're doing" stuff will stay protected, in the parent.

public class Save
{
    public static void SetInt(string key, int value)
    {
        SetInt(currentSaveData, key, value);
    }

    // ancillary / dangerous methods are protected
    protected static void SetInt(object dataBacking, string key, int value) { /* ... */ }
    protected static void StopAllSaves() { /* ... */ }
}

public class SaveDataMigration : Save // special-case code can go in subclasses/adapters
{
    public static void ReplaceSaveData()
    {
        StopAllSaves(); // adapter can access the dangerous methods
        // ...
    }
}

*_{Why does the API need to be static? This is one of those few cases where keystrokes matter. When adding new parts to the game, I need mental energy free for the hard problems. Saving object data is boilerplate, often written in the middle of working on a hard problem. That little bit of time and effort to write the name of a singleton instance variable is a bit of mental effort that'll be diverted from the hard problem.}

Answer 4

What you can do at the level of your Save class (with the additional benefit of making those appear as instance methods) is to declare a bunch of Func<...> and/or Action<...> member delegates (say, as public readonly fields that you can set in a constructor only) of same names and signatures as their Impl counterparts then in one of the Save's constructors, you bind them (thanks to some protected void BindTo(Type implType) helper of Save) to the actual implementation found in Impl, with BindTo(...) using:

Delegate.CreateDelegate(Type delegateType, MethodInfo methodInfo)

(if you need to use reflection and not just hard code it in BindTo(...))

In Save, you'll probably want to make that BindTo(...) future proof by declaring it as protected virtual...

Note though, if Save allows for partial binding of itself to Impl, a client of Save's public interface will have the responsibility to check whether whatever delegate it is interested in, is actually bound (not null) or not.

'HTH,