C# repeating lines of code vs using properties from a memory usage perspective

Question

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]);
        }
    }

Collapsing repeated code blocks into a general-purpose function certainly seems like a good approach. Do you have any reason why it wouldn't be? — David, Mar 04 '14 at 15:47
You could use an enum if you don't trust incoming strings, but for your question, its fine the way it is — Jonesopolis, Mar 04 '14 at 15:50
You have a lot of repeating code, so collapsing it into general purpose, and then have accessor methods would help from maintenance persepctive. Code size would probably be smaller though insignificantly... — LB2, Mar 04 '14 at 15:52

score 1 · Accepted Answer · answered Mar 04 '14 at 15:53

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]);
        }
    }
}

score 0 · Answer 2 · answered Mar 04 '14 at 15:53

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.

score 0 · Answer 3 · answered Mar 04 '14 at 15:54

0

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.

answered Mar 04 '14 at 15:54

J0e3gan

8,740
10
53
80

And as others pointed out, be sure that the new method is private and your gutted per-property methods that call it remain public. I figured this goes without saying, but it is probably worth noting. – J0e3gan Mar 04 '14 at 16:00

C# repeating lines of code vs using properties from a memory usage perspective

3 Answers3