views:

248

answers:

6

I am refactoring a large and complicated code base in .NET that makes heavy use of P/Invoke to Win32 APIs. The structure of the project is not the greatest and I am finding DllImport statements all over the place, very often duplicated for the same function, and also declared in a variety of ways:

The import directives and methods are sometimes declared as public, sometimes private, sometimes as static and sometimes as instance methods. My worry is that refactoring may have unintended consequences but this might be unavoidable.

Are there documented best practices I can follow that can help me out?

My instict is to organize a static/shared Win32 P/Invoke API class that lists all of these methods and associated constants in one file... EDIT There are over 70 imports to the user32 DLL.

(The code base is made up of over 20 projects with a lot of windows message passing and cross-thread calls. It's also a VB.NET project upgraded from VB6 if that makes a difference.)

+1  A: 

What I typically try to do in this case is to do what you are talking about, create various classes, static or not, that provide the functionality, this way it can be re-used as needed. Depending on the nature of the calls, I'd shy way from a static class implementation, but that will depend on your specific implementation.

Expansion on Above as requested.

Given the nature of P/Invoke, especially if a number of calls are needed and are of varying areas of implementation I find it better to group like items together, this way you are not pulling in a lot of other clutter, or other DLL imports when not needed.

THe desire to stay away from static methods, is due to calls to unmanaged resources and potential for memory leaks etc..

Mitchel Sellers
Mitchel, would you mind expanding on your answer? Why would you create various classes instead of one? And why would static implementation be less preferable?
Paul Sasik
Updated per request
Mitchel Sellers
+2  A: 

Why not create a singular file called Win32.vb and within that logically group the pinvokes into separate namespaces, for instance a GDI namespace could use all GDI pinvokes, User32 namespace could use all pinvokes that resides in the User32 kernel, and so on....it may be painful at first, but at least you will have a centralized namespaces all contained within that file? Have a look here to see what I mean...What do you think?

tommieb75
+1 Great link! Definitely a great example of the type of thing I might want to do.
Paul Sasik
+3  A: 

Are your P/Invoke calls an artifact of the migration from VB6? I have migrated 300,000 lines of code from VB6 to C# (Windows.Forms and System.EnterpriseServices), and eliminated all but a handful of P/Invokes calls--there is nearly always a managed equivalent. If you are refactoring, you may want to consider doing something similar. The resulting code should be fair easier to maintain.

ShellShock
Not only easier to maintain long run, but also more performant.
Nick
Some of the calls are artifacts and some aren't. I am indeed replacing w/ managed code but a significant amount of p/invoke will have remain.
Paul Sasik
That doesn't help perf, the managed method still P/Invokes.
Hans Passant
Even the P/Invokes that were in the original VB6 code (Declares) can often be replaced by a managed equivalent.
ShellShock
@nobugs - It depends on the method. Besides... constantly marshalling back and forth has to have a cost associated with it.
Nick
+1  A: 

The recommended way is to have a NativeMethods class per assembly with all the DllImported methods in it, with internal visibility. In this manner you know always where your imported function are and avoid duplicate declarations.

munissor
Sounds interesting. Could you provide a link or two to some sources.
Paul Sasik
FxCop suggests to do so.. have a look at http://blogs.msdn.com/fxcop/archive/2007/01/14/faq-how-do-i-fix-a-violation-of-movepinvokestonativemethodsclass.aspx
munissor
+1  A: 

You might consider the way it was done in the .NET framework. It invariably declares a static class (Module in VB.NET) named NativeMethods that contain the P/Invoke declarations. You could be more organized then the Microsoft programmers, there are many duplicate declarations. Different teams working on different parts of the framework.

However, if you want to share this among all projects you have to declare these declarations Public instead of Friend. Which isn't great, it ought to be an implementation detail. I think you can solve that by re-using the source code file in every project that needs it. Normally taboo but okay in this case, I think.

I personally declare them as needed in the source code file that needs them, making them Private. That also really helps when lying about the argument types, especially for SendMessage.

Hans Passant
Thanks all for the great discussion. Too bad I can. Only pick one winner. Thanks nobugz for the very practical considerations.
Paul Sasik
A: 

Organize them into a [Safe|Unsafe]NativeMethods class. Mark the class as internal static. If you need to expose them to your own assemblies, you can use InternalsVisibleTo - though it'd be more appropriate if you could group related ones into each assembly.

Each method should be static - I honestly wasn't aware you could even mark instance methods with DllImport.

As a first step - I'd probably move everything to a Core assembly (if you have one), or create a Product.Native assembly. Then you can find dupes and overlaps easily, and look for managed equivalents. If your p/invokes are a mess, I don't suspect you have much in the way of layering in the other assemblies that will guide your grouping.

Mark Brackett