5
\$\begingroup\$

I'd like to make sure I'm following some basic coding best practices since I've had only little practice creating my own classes and using pinvoke. I've found this class to be pretty useful and I'd like to put it up online but not if it's a poorly written or could cause issues I haven't considered.

I would love any feedback or suggestions for things I should review/revise or completely redo.

/// <summary>
/// Description:  Class for the use of RegQueryInfoKey pinvoke interop in C#.
/// Usage Example: DateTime dTime = RegQuery.lastWriteTime("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
/// </summary>

using System;
using System.Runtime.InteropServices;
using System.Text;

namespace WindowsForm
{
    public class RegQuery
    {
        public const int KEY_QUERY_VALUE = 0x1;
        static UIntPtr hKey = (UIntPtr)0x80000002;
        static UIntPtr hKeyVal;
        static StringBuilder classStr = new StringBuilder(255);
        static uint classSize = (uint)classStr.Capacity + 1;
        static uint lpcSubKeys;
        static uint lpcbMaxSubKeyLen;
        static uint lpcbMaxClassLen;
        static uint lpcValues;
        static uint lpcbMaxValueNameLen;
        static uint lpcbMaxValueLen;
        static uint lpcbSecurityDescriptor;
        static long lpftLastWriteTime;

        [DllImport("advapi32.dll", EntryPoint = "RegOpenKeyEx")]
        extern private static int RegOpenKeyEx_DllImport(UIntPtr hKey, string lpSubKey, uint ulOptions, int samDesired, out UIntPtr phkResult);

        [DllImport("advapi32.dll")]
        extern private static int RegQueryInfoKey(
            UIntPtr hkey,
            StringBuilder lpClass,
            ref uint lpcbClass,
            IntPtr lpReserved,
            out uint lpcSubKeys,
            out uint lpcbMaxSubKeyLen,
            out uint lpcbMaxClassLen,
            out uint lpcValues,
            out uint lpcbMaxValueNameLen,
            out uint lpcbMaxValueLen,
            out uint lpcbSecurityDescriptor,
            out long lpftLastWriteTime
        );

        public static void doQuery(string fullKey)
        {
            string[] hive = fullKey.Split(new char[] { '\\' }, 2);
            if (String.Equals(hive[0], "HKEY_LOCAL_MACHINE", StringComparison.OrdinalIgnoreCase) || String.Equals(hive[0], "HKLM", StringComparison.OrdinalIgnoreCase))
                hKey = (UIntPtr)0x80000002;
            else if (String.Equals(hive[0], "HKEY_CURRENT_USER", StringComparison.OrdinalIgnoreCase) || String.Equals(hive[0], "HKCU", StringComparison.OrdinalIgnoreCase))
                hKey = (UIntPtr)0x80000001;
            else if (String.Equals(hive[0], "HKEY_CLASSES_ROOT", StringComparison.OrdinalIgnoreCase) || String.Equals(hive[0], "HKCR", StringComparison.OrdinalIgnoreCase))
                hKey = (UIntPtr)0x80000000;
            else if (String.Equals(hive[0], "HKEY_USERS", StringComparison.OrdinalIgnoreCase) || String.Equals(hive[0], "HKU", StringComparison.OrdinalIgnoreCase))
                hKey = (UIntPtr)0x80000003;
            else if (String.Equals(hive[0], "HKEY_CURRENT_CONFIG", StringComparison.OrdinalIgnoreCase) || String.Equals(hive[0], "HKCC", StringComparison.OrdinalIgnoreCase))
                hKey = (UIntPtr)0x80000005;

            RegOpenKeyEx_DllImport(hKey, hive[1], 0, KEY_QUERY_VALUE, out hKeyVal);
            RegQueryInfoKey(hKeyVal, classStr, ref classSize, IntPtr.Zero, out lpcSubKeys, out lpcbMaxSubKeyLen, out lpcbMaxClassLen, out lpcValues, out lpcbMaxValueNameLen, out lpcbMaxValueLen, out lpcbSecurityDescriptor, out lpftLastWriteTime);
        }

        /// <summary>
        /// A pointer to a buffer that receives the user-defined class of the key.
        /// Example: int cString = DateTime dTime = RegQuery.classString("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
        /// </summary>
        public static string classString(string fullKey)
        {
            doQuery(fullKey);
            return classStr.ToString();
        }
        /// <summary>
        /// A pointer to a variable that receives the number of subkeys that are contained by the specified key.
        /// Example: uint sKeys = RegQuery.subKeys("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
        /// </summary>
        public static uint subKeys(string fullKey)
        {
            doQuery(fullKey);
            return lpcSubKeys;
        }
        /// <summary>
        /// A pointer to a variable that receives the size of the key's subkey with the longest name, in Unicode characters, not including the terminating null character. 
        /// Example: uint mSubKeyLen = RegQuery.maxSubKeyLen("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
        /// </summary>
        public static uint maxSubKeyLen(string fullKey)
        {
            doQuery(fullKey);
            return lpcbMaxSubKeyLen;
        }
        /// <summary>
        /// A pointer to a variable that receives the size of the longest string that specifies a subkey class, in Unicode characters. The count returned does not include the terminating null character. 
        /// Example: uint mClassLen = RegQuery.maxClassLen("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
        /// </summary>
        public static uint maxClassLen(string fullKey)
        {
            doQuery(fullKey);
            return lpcbMaxClassLen;
        }
        /// <summary>
        /// A pointer to a variable that receives the number of values that are associated with the key.
        /// Example: uint vals = RegQuery.values("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
        /// </summary>
        public static uint values(string fullKey)
        {
            doQuery(fullKey);
            return lpcValues;
        }
        /// <summary>
        /// A pointer to a variable that receives the size of the key's longest value name, in Unicode characters. The size does not include the terminating null character. 
        /// Example: uint mValueNameLen = RegQuery.maxValueNameLen("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
        /// </summary>
        public static uint maxValueNameLen(string fullKey)
        {
            doQuery(fullKey);
            return lpcbMaxValueNameLen;
        }
        /// <summary>
        /// A pointer to a variable that receives the size of the longest data component among the key's values, in bytes.
        /// Example: uint mValueLen = RegQuery.maxValueLen("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
        /// </summary>
        public static uint maxValueLen(string fullKey)
        {
            doQuery(fullKey);
            return lpcbMaxValueLen;
        }
        /// <summary>
        /// A pointer to a variable that receives the size of the key's security descriptor, in bytes.  
        /// Example: uint sDesc = RegQuery.securityDescriptor("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
        /// </summary>
        public static uint securityDescriptor(string fullKey)
        {
            doQuery(fullKey);
            return lpcbSecurityDescriptor;
        }
        /// <summary>
        /// A pointer to a FILETIME structure that receives the last write time. 
        /// Example: DateTime dTime = RegQuery.lastWriteTime("HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox");
        /// </summary>
        public static DateTime lastWriteTime(string fullKey)
        {
            doQuery(fullKey);
            return DateTime.FromFileTime(lpftLastWriteTime);
        }

    }
}
\$\endgroup\$
  • \$\begingroup\$ Is there a reason you're not using Microsoft.Win32.Registry? \$\endgroup\$ – Ben Aaronson Jul 30 '14 at 1:11
  • \$\begingroup\$ Win32 isn't able to return the last write time of a key. That is the primary use I made it for. I'd have to check but I don't think it has access to the class info embedded in the keys as well. \$\endgroup\$ – KevinD Jul 30 '14 at 1:13
4
\$\begingroup\$

Scope of methods and const

There is no reason for KEY_QUERY_VALUE to has a public scope. The same is true for the doQuery() method.

Naming

Following the naming guidlines you should use PascalCasing for names of classes, structs and methods. See also: https://stackoverflow.com/a/1618325/2655508

So

public static void doQuery(string fullKey)

will become ( with casing and scope )

private static void DoQuery(string fullKey)

and

public static uint subKeys(string fullKey)

will become

public static uint SubKeys(string fullKey)

but this is more named like a property. A method should be named as a verb or a verb phrases. It also reads in the comments to this method ...that receives the number of subkeys... , so a better name would be GetNumberOfSubKeys. This is also true for your other methodnames.

Braces

It is best practice to use braces for if..else, for.. etc. everytime, also if it would not be neccessary. See: https://codereview.stackexchange.com/a/49212/29371

So the if..else construct in the DoQuery() method would look like

if (String.Equals(hive[0], "HKEY_LOCAL_MACHINE", StringComparison.OrdinalIgnoreCase) || String.Equals(hive[0], "HKLM", StringComparison.OrdinalIgnoreCase))
{
    hKey = (UIntPtr)0x80000002;
}
else if (String.Equals(hive[0], "HKEY_CURRENT_USER", StringComparison.OrdinalIgnoreCase) || String.Equals(hive[0], "HKCU", StringComparison.OrdinalIgnoreCase))
{
    hKey = (UIntPtr)0x80000001;
}
.....

Also it is only a matter of taste, i prefer switch..case over if..else if .

Validation

You should validate the inputparameter of the public methods for correctness. Assume a user of this class will call one of these methods with a parameter like

"\\HKEY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox"

or

null

or

"HKEYY_Current_user\\software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Dropbox"

GetLastError

While pinvoking native dlls, you should always check, if the call to one of the native methods,results in an error.

You can do this by setting the attribute SetLastError=true like

[DllImport("advapi32.dll",, SetLastError=true, EntryPoint = "RegOpenKeyEx")]
    extern private static int RegOpenKeyEx_DllImport(UIntPtr hKey, string lpSubKey,
                              uint ulOptions, int samDesired, out UIntPtr phkResult);

and calling Marshall.GetLastWin32Error . See also: https://stackoverflow.com/a/17918729/2655508

\$\endgroup\$
  • \$\begingroup\$ I'm not sure you can really call it "best practice" to always use braces with if statements, I've heard advice either way. For example, if the line is short, you can write it all on one line like: if(condition) DoThing();, which doesn't have any risk of maintainers making mistakes when adding to it \$\endgroup\$ – Ben Aaronson Jul 30 '14 at 8:40
  • \$\begingroup\$ I appreciate all the great suggestions and the time you've taken to look the code over. I'm going to get started making these changes before I put it on Codeplex. \$\endgroup\$ – KevinD Jul 31 '14 at 0:07

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.