2

I'm having a bit of a moment of indecision here and I'd like some perspective on it.

I'm currently wrapping up objects from a 3rd party API (at home for my own project and at work), and I'm doing my best to adhere as closely to SOLID principles as I can. All of this is written using C#. Just to be clear here: this is not a line of business application, I'm making a framework for reuse that wraps up more complicated details of a 3rd party API.

So my issue is this: I have these objects wrapped up in my concrete instances. We'll call the native objects that are wrapped nA and nB, and the wrappers A and B which hold nA and nB respectively:

class A { private nA _nA }
class B { private nB _nB }

nB can't be created without passing a reference to nA. So, to avoid exposing the nA object to the public, I have a factory method to create wrapper B so it can pass in nA to the constructor of nB. So, first question: I'm assuming this is a proper practice? I don't want to end up with a ton of factories or factory methods if I can help it (there's a fair bit to wrap up).

Now, this is all well and good. I can create A and B no problem because of the above. But, now I have another wrinkle. I have to wrap a method called M1(nB) where the parameter is of type nB, which is stored inside of the B wrapper:

public class A { private nA _nA; public M1(B) { _na.M1( ??? );} }

But, nB is inaccessible. So, what's the best practice to pass nB to the nA.M1() method stored in wrapper A?

I could enforce the use of concrete types, and make nB an internal member on wrapper B like so:

public class B { internal nB { get; private set; } }

But this will introduce tight coupling. Does it matter since nA and nB are tightly coupled anyway?

These kinds of problems always give me a headache, so if anyone could spare some advice, that'd be swell.

Also, while I am trying to adhere to SOLID as best as possible, I am aware they are guidelines. They are not hard set rules or a religion after all. So if I have no choice but to break SOLID principles to get what I need, so be it.

Some code to further illustrate my issue:

interface IGraphics
{
   CopyResource(Texture src, Texture dest);
}

interface ITexture
{
   // Stuff for textures that have no bearing on this example.
}

class Graphics
   : IGraphics
{
   // Native object (or object that wraps a native object).
   private ID3D11Context1 _d3dContext;

   public CopyResource(Texture src, Texture dest)
   {
      // Do validation, etc...    

      // How can I do this with interfaces?  
      // Internals are not allowed on interfaces by the language
      _d3dContext.CopyResource(src._d3dTexture, dest._d3dTexture);
   }
}

class Texture
   : ITexture
{
   // Native object (or object that wraps a native object).
   private ID3D11Texture2D _d3dTexture;
   // Other stuff
}
  • I realize you are just providing an example, but it's very hard to comprehend the problem, and to suggest solutions, with symbols like nA _na M1 and IB. Is there any concrete example you can provide that is similar to a real-world problem? – John Wu Apr 10 '18 at 2:25
  • I did that brevity. I did not want to dump in reams of code for this. But I think can give a real world example. I'll update the code. – Mike Apr 10 '18 at 2:35
  • What should happen if I have D3DGraphics g; OpenGLTexture t1; VulkanTexture t2; and I try to do g.CopyResource(t1, t2);? I get some kind of error that they aren't D3D textures, right? So you can check that they're D3DTextures, and then just cast to D3DTexture. – immibis Apr 11 '18 at 3:34
  • Related: if I have two different IGraphics can I interchange ITextures between them? Or can each ITexture only be used with the IGraphics that it was generated by? – immibis Apr 11 '18 at 3:35
  • @immibis, Well your first comment will never happen as I tend to stick to one API. But casting like that and throwing an exception on the wrong type is a SOLID violation. – Mike Apr 11 '18 at 4:52
1

But this will introduce tight coupling. Does it matter since nA and nB are tightly coupled anyway?

No, it doesn't matter as long as they are internal classes, accessed publicly via interfaces. It's like what goes on inside your classes: if Foo calls a private method, Bar, it's tightly coupled to Bar. But if you later break that coupling and change it to call FooBar and Baz, nothing else is affected as the coupling is encapsulated within your class.

The same applied to what goes in inside A and B. As long as the public methods reference IA and IB only, the internals of A and B are free to intertwine themselves with nA and nB. In fact, not only are they free to do this, they are expected to: it's their job as wrappers.

Just make sure that those wrappers are exposed to the rest of the system via interfaces, IA and IB. That way, the wrappers (and thus the 3rd party libraries) can be mocked when needed. If you don't do that, your wrappers aren't doing anything useful as the rest of the code is still coupled to nA and nB, via an obfuscation layer, rather than an abstraction layer.

Update

Looking at your updated code example, I'm going to back track a bit, as interfaces may be more trouble here than they are worth. When wrapping classes like this, I personally follow the following "rules":

  1. Try to expose all of the wrapped classes via just one interface, encapsulating all the data types etc behind the wrapper. This is the ideal situation, but rarely is practical. It won't work here.
  2. Ensure that, eg in your case ITexture, defines enough properties to allow you to create a Texture (and thus a ID3D11Texture2D) from any implementation of ITexture, so that you can do things like:

    public CopyResource(ITexture src, ITexture dest)
    {
        // Do validation, etc...    
    
       _d3dContext.CopyResource(CreateRealTexture(src).D3dTexture,
                                CreateRealTexture(dest).D3dTexture);
    }
    

    with D3dTexture then being an internal property.

    That then allows you to still hide all the real data types, only exposing them via interfaces. The downsides to this are that it can make the interfaces complex and there's a big overhead in creating the "real" types on the fly. This again is unlikely to be of use to you here.

  3. Do away with interfaces and use factories to create instances, as you suggest. At this point though, I'd say stop and ask yourself "why?" What is the point of such wrappers? They are a lot of work to create and maintain. They don't help much with testing, DI etc (due to the lack of interfaces).

    If the reason for creating them is so that "I can swap out the graphics library for a different one, as I've added an abstraction to their access". Then I'd say, don't waste your time. The chances of a different graphics library being able to slot in behind your wrapper classes with no changes to those wrappers is vanishingly small. Just access the libraries directly and rewrite if you change.

So there's no perfect solution here. Ask yourself if the wrappers are genuinely of value. If they are, then my gut reaction in this particular case would be to change the likes of:

private ID3D11Context1 _d3dContext;

to be internal items. That way, behind the scenes, your wrapper classes can access all of the underlying types with ease.

  • OK, I understand that interfaces are important for mocking and unit testing. But I have an additional problem here, and maybe I've misread your answer, but I have to pass in wrapper B to wrapper M1 to call nA.M1(), if wrapper B is an interface, how do I get the internal reference to nB? – Mike Apr 9 '18 at 15:14
  • I've updated the question with some code to illustrate what I'm after here. – Mike Apr 9 '18 at 15:21
  • @Mike, I started updating my answer, but realised I'm confused over the details. You appear to be saying that nB is passed a nA via the constructor. But nA references nB instances via eg the M1() method. Is that correct? Is that multiple instances of nB, or is there a one to one relationship between them? – David Arno Apr 9 '18 at 16:18
  • I apologize, my source code example was incorrect, I've since updated it. As for the constructor parameter passing, I said "I have a factory method to create wrapper B so it can pass in nA to the constructor of nB". The API I'm wrapping is full of stuff like this. – Mike Apr 9 '18 at 16:42
  • 1
    I've read your update. The reason I wrap it up is so I can control the state information (sampler, blend, etc...). My framework may expose some direct methods, but how it renders is way different than how an end user would have to do it if they used D3D directly. If I just made a thing that did only certain operations, then the state is at the user's mercy and thus I could not make the same guarantees. Anyway, your final bit there is what I figured would be the case, so I've marked yours as the answer. Thank you. – Mike Apr 10 '18 at 14:08
0

Three thoughts for you, OP.

Use an anemic class

If B is just a DTO, there is no reason to be able to mock it, and an interface is not needed (a unit test can just use new and put in mock data). For example, if B is a custom structure designed to hold a date/time stamp, and it contains only fields and properties without behavior, go ahead and expose it as class B instead of as interface IB. It's OK for the client to use new instead of getting every single instance from your library, and unit tests can do the same, as long as the class is a dumb data container.

Use an ID to reconstruct the original object

Don't worry about keeping like-for-like with every single pattern in use by the objects you are wrapping. The point of a wrapper is to expose only what is needed by your client, and in a manner that is easy to use and difficult or impossible to mis-use. It's OK if things change.

For example, it might make more sense to expose a method that accepts an object identifier than an object itself. So instead of

void Method1(ICustomer customer);

you could expose

void Method1(int customerID);

Now internally, method1 will have to use that customerID to construct an (internal) Customer object, compatible with the library you are attempting to wrap. I would suggest it can just construct it on the fly.

void Method1(int customerID)
{
    var c = internalLibrary.GetCustomer(customerID);
    c.InternalLibraryCall();
}

Use internal interfaces

You have a powerful tool at your disposal: internal interfaces. Not internal interface members (as you have discovered, these are not allowed) but the interface itself.

public interface ICustomer
{
    int CustomerID { get; }
}

internal interface ICustomerEx
{
    InternalCustomer GetOriginalCustomerObject();
}

public class Customer: ICustomer, ICustomerEx  //Client won't see ICustomerEx
{
    InternalCustomer _customer;

    internal Customer(InternalCustomer c)
    {
        _customer = c;
    }

    InternalCustomer ICustomerEx.GetOriginalCustomerObject()
    {
        return _customer;
    }
}

Then the method could look like this:

public void HandleCustomer(ICustomer customer)
{
    var c = customer as ICustomerEx;
    var internalC = c.GetOriginalCustomerObject();
    c.InternalLibraryCall();
}
  • Unfortunately no, B is not a lightweight class, none of them are. As for the ID and construction on the fly, yeah, that's definitely not going to work for me since I'm not creating anything by passing M1(B). And finally, using an internal interface had occurred to me, but that looks like a violation of Open/Close (for example, if I were to test HandleCustomer with my own mocked interface, it'd die on the table). I probably should have mentioned it, but this code is not for a LOB application, but a framework/API (I'll update my question). – Mike Apr 10 '18 at 2:20
  • "B is not a lightweight class." Isn't it your wrapper? You can write it any way you want. Again, it does not have to work exactly the same as the library that you are wrapping. – John Wu Apr 10 '18 at 2:23
  • It is my wrapper and B is not a real class either. The point of these hypothetical types is to try and get an idea of how I can do what I'm asking and still keep it testable/SOLID. At this point, it's really starting to look like I have to go the concrete and internal route. My actual code is using Direct 3D, and performance is paramount. For example, if I need to call CopyResource (a method I'd absolutely need), I need to call it on the main interface (ID3D11Context) and pass in a D3D resource object. Both of these internal types are wrapped up and not exposed to the world. – Mike Apr 10 '18 at 2:33
  • Oh...!!! OK, yes, that is very different from what I was thinking. You definitely need to hang onto the original object then, I believe, since it ultimately wraps an unmanaged resource. Is that correct? – John Wu Apr 10 '18 at 2:40
  • You got it. Please look at my updated example source and it'll hopefully be more clear. – Mike Apr 10 '18 at 2:41

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.