6

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).

  • 1
    These are examples of Single Responsibility Principle being broken - that's why you would like them to be "hidden from the normal view". – rwong Mar 10 '16 at 7:48
  • @rwong: There is only one responsibility being illustrated here: Adapter. If the goal is to re-organize the responsibilities of an existing API hierarchy, that's a different kettle of fish, but the OP never mentioned SRP, and keeping the new API consistent with the original seems prudent. – Robert Harvey Mar 10 '16 at 7:49
  • @piojo I'm not sure how the samples you gave live up to the premise that ancillary/dangerous functions should be hidden. Normal classes still have access to them, unlike private functions in C++, right ? In the end, is this still a requirement you have ? – guillaume31 Mar 10 '16 at 8:07
  • 1
    @Piojo Have you thought about making them internal? For this kind of work with low level APIs you often find yourself making things internal to separate interdependent functionalities, and separate them entirely from the user. – Snoop Mar 10 '16 at 12:50
  • 1
    @piojo This sort of thing tends to happen a lot when the group responsible for your API does not bother to give you a .NET version and only provides a wrapper file that marshals everything from the native code. Or when you're forced to marshal everything yourself. – Snoop Mar 10 '16 at 13:02
3

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.

  • 1
    Thanks for explaining the normal usages of those language constructs. Using one single adapter class per area of extended responsibility sounds great, but that would still require the low-level APIs to be visible in the main Save class. That's why I split it off into an "implementation details" class, unorthodox as it is. (There's another reason to keep Save's API simple: it's a drop-in replacement for the game engine's save feature, so I copied the API.) – piojo Mar 10 '16 at 9:18
  • 1
    When implementing a facade pattern to wrap unsafe code, you could potentially find yourself using a lot of "internal" to separate mixing/inter-dependent functionalities that make the pure/original API hard to use. You see this a lot in test/hardware engineering. – Snoop Mar 10 '16 at 12:25
3

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.

1

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.

  • Note: the disadvantage I've found with this method is that the adapter classes will also contain the base class's public API. If desired, it could be fixed by putting no public methods in the base, and instead putting the public API in a new adapter. That way only details/data would be part of the base class, and every group of public methods would live in an adapter class (including the safe and commonly used functions). – piojo Mar 16 '16 at 4:37
  • Your rationale for why the API needs to be static is a dumb one. "i'm out of brain" is going to come back and make your life harder later (now). – Bryan Boettcher Dec 2 '16 at 20:58
  • @inst Wiser programmers than us have disagreed. Unreasonably short function invocations are the norm for a lot of really important functions in various programming languages. Plus, I'm writing a library to replace a different static API. The burden of proof does not lie on me if I'm arguing for keeping the original ubiquitous calling convention. – piojo Dec 5 '16 at 10:43
0

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,

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.