1

I'm writing a library to be used by several applications.

Some interfaces which my library declares and uses, and which are implemented by the application, look something like this:

interface IOpaqueHandle : IDisposable {}

interface IPaintFactory
{
    IOpaqueHandle Create(string configData);
}

interface IPainter
{
    void Paint(IOpaqueHandle handle);
}

The Create method with configData parameter tells the application to allocate some resources and return a handle to those resources:

  • My library doesn't know what types of resource are allocated by the application, nor exactly what methods those resources support.

  • The resources probably come from various 3rd-party application-specific library, known to the application but not known by my library.

  • Create will be called multiple times with different parameter values, to allocate different resources each with their own IOpaqueHandle instance

The Paint method is an example of telling the application to do something with a specified resource.


My question is, what's the best way to implement the handle-to-resource mapping in the application?

For example, assuming that the application's resources are defined by a class named Resources, I can think of two ways to implement it.

  1. Using a subclass with an upcast:

    class Resource : IOpaqueHandle
    {
        internal static Resource getResource(IOpaqueHandle handle)
        {
            return (Resource)handle;
        }
    }
    
  2. Using a dictionary to map from the handle to the object:

    class Resource : IDisposable
    {
        internal static Resource getResource(IOpaqueHandle handle)
        {
            return s_dictionary[handle];
        }
    
        class Handle : IOpaqueHandle
        {
            public void Dispose()
            {
                Resource resource = getResource(this);
                resource.Dispose();
                s_dictionary.Remove(this);
            }
        }
    
        static Dictionary<IOpaqueHandle, Resource> s_dictionary =
            new Dictionary<IOpaqueHandle, Resource>();
    
        internal static IOpaqueHandle Create(string configData);
        {
            Handle handle = new Handle();
            Resource resource = new Resource(configData);
            s_dictionary.Add(handle, resource);
            return handle;
        }
    
        Resource(string configData) { ... }
    }
    

Is the subclass with an upcast simpler (to implement) and faster (at run-time) and therefore preferable?

Does the second solution have any advantages? It avoids an upcast, for what's that worth -- is that good?


(First edit)

You might suggest declaring Paint as a method of the handle interface -- BUT that's not possible because of different lifetimes:

  • An IOpaqueHandle is a long-lived object, e.g. created once at object startup
  • An IPainter is a short-lived object, e.g. a new one is created each time the O/S window needs repainting.

A short-lived IPainter instance is created by the application and passed-in to my library, which calls its Paint method. The implementation of IPainter contains some short lived resources. The Paint method must combine the short-lived resources (contained in the IPainter) with the long-lived resources (contained in the handle).

Theoretically I could declare a method like this, instead of putting the Paint method in the IPainter interface:

interface IOpaqueHandle
{
    // paint using short-lived resources contained in IPainter
    void Paint(IPainter painter);
}

... but that amounts to the same problem, i.e. how to extract implementation-specific resources from the opaque IPainter interface.


(Second edit)

At the risk of making this question too long, here's some sample code.

The following code is in the library, in addition to the interfaces declared at the top.

class Node
{
    internal readonly IOpaqueHandle handle;

    // plus a lot of other data members
    // e.g. to determine whether this node is visible and should be painted

    internal Node(string configData, IPaintFactory factory)
    {
        // get the handle which we'll pass back to the application if we paint this node
        handle = factory.Create(configData);
    }

    internal bool IsVisible { get { ... } }
}

// facade
public class MyLibrary
{
    List<Node> nodes = new List<Node>();

    // initialize data using config data,
    // and using app-specific paint factory passed-in by application
    public void Initialize(List<string> configStrings, IPaintFactory factory)
    {
        configStrings.ForEach(configData => nodes.Add(new Node(configData, factory)));
    }

    // plus a lot of other methods to manipulate the Nodes

    // called from application when its window wants repainting
    public void OnPaint(IPainter painter)
    {
        nodes.ForEach(node => { if (node.IsVisible) painter.Paint(node.handle); });
    }
}

The above is a simplification. Because Node and MyLibrary are actually hundreds of classes, it wouldn't be convenient to wrap them all in a single generic class so that they share a common template parameter of type T.

The following code is in the application. All the types in the Os namespace belong to some library which the application is using and which my library doesn't know about.

class Resource : IOpaqueHandle
{
    readonly Os.Font font;
    readonly string text;

    internal Resource(string configData) { ... }

    // app-specific method using app-specific types
    // which my library doesn't know about and which
    // therefore isn't declared in the library's IOpaqueHandle interface
    internal void GraphicalPaint(Os.Graphics graphics)
    {
        graphics.DrawText(this.font, this.text);
    }
}

class PaintFactory : IPaintFactory
{
    public IOpaqueHandle Create(string configData)
    {
        return new Resource(configData);
    }
}

class Painter : IPainter, IDisposable
{
    internal readonly Os.Graphics graphics;

    internal Painter(Os.Graphics graphics)
    {
        this.graphics = graphics;
    }

    public void Dispose() { graphics.Dispose(); }

    public void Paint(IOpaqueHandle handle)
    {
        // use magic in question to retrieve app-specific resource from opaque handle
        Resource resource = (Resource)handle;

        // invoke app-specific method to paint this resource
        resource.GraphicalPaint(this.graphics);
    }
}

class Main : Os.Window
{
    MyLibrary myLibrary = new MyLibrary();

    void initialize(List<string> configStrings)
    {
        PaintFactory paintFactory = new PaintFactory();
        myLibrary.Initialize(configStrings, paintFactory);
    }

    // plus other methods to poke the data in MyLibrary

    // event handler called from Os when Window needs repainting
    void onPaint(Os.Graphics graphics)
    {
        using (Painter painter = new Painter(graphics))
        {
            myLibrary.OnPaint(painter);
        }
    }
}
  • 1
    Why do you need the opaque handle at all? It's not a very object-oriented way of designing a system, and it would probably be easier to use if the Paint method was supplied as a method of the returned object, for example. Even if the object you return doesn't include the methods that use it, it can just delegate to them, at which point you have a useful object rather than a handle. – Jules Feb 25 '18 at 11:23
  • Your code doesn't make sense. What is Resource? What is GetResource? How does the first snippet relate to the two options when names seem completely different? – Euphoric Feb 25 '18 at 11:24
  • @Jules Yes I'm thinking about that. It's because IPainter defines a lot of other methods as well, which don't depend on those allocated resource parameters ... so I put all those painting methods in one interface, instead of putting some of them in the resource-specific interface. – ChrisW Feb 25 '18 at 11:25
  • @Jules I updated the question to explain why I don't think that's possible. – ChrisW Feb 25 '18 at 12:05
  • 1
    Why do you use generic IOpaqueHandler? Why aren't you creating and consuming specific PaintResource? Possibly one that implements IDisposable? – Euphoric Feb 25 '18 at 12:17
1

The Plain answer is that any kind of mapping or up-casting is bad as it risks a run time error when it could otherwise be prevented by strongly typed classes.

Edit: OP included app code.

The solution is Generics, which allow you to specify a Type that your library has no knowledge of.

IPaintFactory<T>
{
    IOpaqueHandle<T> Create(config);
}

IOpaqueHandle<T>
{
    T Resource {get;}
}

This allows the app to specific the type of resource underneath the handler. painter can then be:

App.Painter : IPainter<App.MyResource>
{
    void Paint(IOpaqueHandle<App.MyResource> handle)
    {
        handle.Resource.MySecretPaintMethod(); 
    }
}

In Your case the type you need to use but the library doesn't know about is OS.Graphics.

Because the library class itself has the method which needs this type myLibrary.OnPaint The library will need to be a generic type. The Generic type parameter propagates up to where this function is specified.

Possibly you could refactor so that some sub class of library has the OnPaint method, then you could limit the generics to that sub type.

  • This feels to me like you are trading one evil (runtime casting) with another evil (generics where they don't really fit). Generics might not be usable if he has more than ~3 resource types. Because IPaintFactory would need to have all of them. – Euphoric Feb 25 '18 at 15:04
  • shrug I was going to upvote your answer before it was deleted. I dont think there is enough info to really solve the problem, and if there was it would prob be a redesign of the whole thing, not fiddling with the interfaces – Ewan Feb 25 '18 at 15:19
  • Sorry I don't see how generics helps: the library can't use these interfaces (e.g. it cannot call Create to get a IOpaqueHandle<T>) for any specific T, because the library doesn't know what type T is. The type (e.g. Resource) is known to the application (or subclass), but is not known to the library. Another problem is that in C# (as opposed to C++), I think it's dubious whether Painter can call MySecretPaintMethod on any type T (unless the template specified that T is where some interface that publishes MySecretPaintMethod). – ChrisW Feb 25 '18 at 16:24
  • Generics are indeed the way to go, but the generic type shouldn't be a parameter to IOpaqueHandle, it serves the exact same purpose and therefore replaces it. – Ben Voigt Feb 25 '18 at 16:41
  • 1
    If the library defines a generic interface like interface IOpaqueHandle<T> { T Resource { get; } } it cannot store an instance of that interface e.g. like struct Data { IOpaqueHandle<T> Handle; } unless it specifies what type T is. But the library doesn't know what type T is. For this to work I think the whole library would need to be a generic class of type T. – ChrisW Feb 25 '18 at 16:57
0

OK You did include the critical myLibrary.Onpaint() function, but here's How I would refactor your new code to avoid the needless casting, generics even though it totally solves you issue, and adding the factory to the resource handler.

So this will be solution 3.

I kinda feel you are just going to go "oh that wouldn't work because.. adds more code"

class Main : Os.Window
{
    MyLibrary myLibrary = new MyLibrary();
    PaintFactory paintFactory;
    void initialize(List<string> configStrings)
    {
        paintFactory = new PaintFactory();
        myLibrary.Initialize(configStrings, paintFactory);
    }

    // plus other methods to poke the data in MyLibrary

    // event handler called from Os when Window needs repainting
    void onPaint(Os.Graphics graphics)
    {
        foreach(var r in paintFactory.ResList)
        {
            r.graphics = graphics;
        }

        myLibrary.OnPaint();
    }
}

Edit: Jeeze I missed the nodes. OK well you just have to switch it around and set the Os.Graphics on each resource

public void OnPaint()
{
    nodes.ForEach(node => { if (node.IsVisible) node.handle.Paint(); });
}

class Resource : IOpaqueHandle
{
    readonly Os.Font font;
    readonly string text;

    internal Resource(string configData) { }

    public void Dispose()
    {
        throw new NotImplementedException();
    }

    // app-specific method using app-specific types
    // which my library doesn't know about and which
    // therefore isn't declared in the library's IOpaqueHandle interface
    internal void GraphicalPaint(Os.Graphics graphics)
    {
        graphics.DrawText(this.font, this.text);
    }

    public PaintFactory parent;
    public Os.Graphics graphics { get; set; }
    public void Paint()
    {
        GraphicalPaint(graphics);
    }
}

Pretty ugly way to avoid generics though

  • I understand what you're suggesting, thank you, yes, that would work too. – ChrisW Feb 25 '18 at 21:32
0

You can sometimes implement this (i.e. dispatching on two base types to get the leaf types) using "double-dispatch". But I think you can't do that in this case, though, because the base types or interfaces (owned by the library) cannot name the leaf types implemented by the application.

In this case, if you're going to use the Dictionary method to get the leaf Resource type, it would be better (more conventional) to use a value type as the Dictionary key, instead using of a methodless interface as an opaque handle (i.e. using a reference type as a dictionary key).

  • Name the value resourceId or something like that.
  • The type of the value could be int or Guid; or, for type-safety and to help make the API self-documenting, a custom value type like struct ResourceId { int idValue; ... }.

So:

interface IPaintFactory
{
    // create the resource and return a resource id
    int Create(string configData);
}

interface IPainter
{
    void Paint(int resourceId);
}

I'm not sure what the argument against upcasting (instead of using a Dictionary) is.

  • It seems safe enough -- if the application is the only software in the process that's creating objects derived from IOpaqueHandle i.e. Resource objects, then surely it's safe to assume that every IOpaqueHandle instance isa Resource instance.

  • And I guess that upcasting might be microscopically faster than doing a dictionary lookup.

  • And the Dictionary needs to be maintained by the application, and the lifetime of dictionary entries needs to be managed -- with the "handle" method, the library stores the handles and can Dispose each handle when its no longer needed, instead of the library storing IDs (keys) and using another custom API (other than Dispose) to tell the application to clear the Dictionary entry and dispose the corresponding Resource.

In the end though a dictionary with an ID may be better even if it's theoretically worse (slower and more complicated) than upcasting from a handle -- because apparently some programmers think that upcasting is the devil, and assume that if you're doing it then you're doing it wrong and there must be some alternative.

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.