質問

I'm wondering if there's a better way to implement the following code from a maintainability and a memory impact standpoint. Looking at the following code would it be better to use properties? I see a lot of repeat code and I'm wondering what the best way to go about this would be.

public class Win32OperatingSystem
{
    internal ulong BytesToMegaBytes(ulong bytes)
    {
        return bytes / (ulong)1024;
    }

    public ulong FreePhysicalMemory()
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT FreePhysicalMemory FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current["FreePhysicalMemory"]);
        }
    }

    public ulong TotalVirtualMemorySize()
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT TotalVirtualMemorySize FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current["TotalVirtualMemorySize"]);
        }
    }

    public ulong FreeVirtualMemory()
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT FreeVirtualMemory FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current["FreeVirtualMemory"]);
        }
    }
} 

Rather than repeat the method would it be better to do something like...

MyMethod(string propertyName)
{
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current[propertyName]);
        }
    }
役に立ちましたか?

解決

Using the DRY principle would make your second option the better one (Ignoring the lack of SQL parameters in this instance). Its less code to write and would be easy to maintain. However it could lead to a bug if the developer fixing the code didn't know all the places that called this function.

I would lean to having the single private method and then calling that from the public methods.

public class Win32OperatingSystem
{
    internal ulong BytesToMegaBytes(ulong bytes)
    {
        return bytes / (ulong)1024;
    }

    public ulong FreePhysicalMemory()
    {
        return GetProperty("FreePhysicalMemory");
    }

    public ulong TotalVirtualMemorySize()
    {
        return GetProperty("TotalVirtualMemorySize");
    }

    public ulong FreeVirtualMemory()
    {
        return GetProperty("FreeVirtualMemory");
    }

    private ulong GetProperty(string propertyName)
    {
        ManagementObjectSearcher moSearcher = new ManagementObjectSearcher
            ("SELECT " + propertyName + " FROM Win32_OperatingSystem");
        using (var enu = moSearcher.Get().GetEnumerator())
        {
            if (!enu.MoveNext()) return 0;
            return BytesToMegaBytes((ulong)enu.Current[propertyName]);
        }
    }
}

他のヒント

Of course, that's what the DRY principle is about.

But don't replace the other 3 methods with that one though. Make your new method private (and while you're at it, give it a better name), and make the other 3 call it.

public ulong FreePhysicalMemory()
{
    return FindMemoryObject("FreePhysicalMemory");
}

This is not only better for maintainability, but also for readability.

As for performance or memory - don't worry. Yes, there's an extra level of abstraction, a method has to be called (and probably won't be inlined), but that's micro-optimization. Go for readability + maintainability.

Memory impact will be negligible, but violating the DRY principle by repeating an implementation across so many methods is not good for maintainability: yes, it would be better to do what you have proposed instead.

ライセンス: CC-BY-SA帰属
所属していません StackOverflow
scroll top