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 ownIOpaqueHandle
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.
Using a subclass with an upcast:
class Resource : IOpaqueHandle { internal static Resource getResource(IOpaqueHandle handle) { return (Resource)handle; } }
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);
}
}
}
3 Answers 3
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– Euphoric2018年02月25日 15:04:29 +00:00Commented Feb 25, 2018 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 interfacesEwan– Ewan2018年02月25日 15:19:09 +00:00Commented Feb 25, 2018 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– ChrisW2018年02月25日 16:24:41 +00:00Commented Feb 25, 2018 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– Ben Voigt2018年02月25日 16:41:12 +00:00Commented Feb 25, 2018 at 16:41 -
1If the library defines a generic interface like
interface IOpaqueHandle<T> { T Resource { get; } }
it cannot store an instance of that interface e.g. likestruct Data { IOpaqueHandle<T> Handle; }
unless it specifies what typeT
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– ChrisW2018年02月25日 16:57:18 +00:00Commented Feb 25, 2018 at 16:57
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– ChrisW2018年02月25日 21:32:37 +00:00Commented Feb 25, 2018 at 21:32
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
orGuid
; or, for type-safety and to help make the API self-documenting, a custom value type likestruct 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 everyIOpaqueHandle
instance isaResource
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 canDispose
each handle when its no longer needed, instead of the library storing IDs (keys) and using another custom API (other thanDispose
) to tell the application to clear theDictionary
entry and dispose the correspondingResource
.
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.
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.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.