Should I refactor for DRY?

I'd like to get some input on whether or not to DRY up this code. And if so how to implement it correctly. How do I know when it's worth refactoring?

```
public class Win32OperatingSystem
{

public Win32OperatingSystem() { }

///
/// Number, in megabytes, of physical memory currently unused and available.
///
public ulong FreePhysicalMemory()
{
using (PropertyGetter getProperty = new PropertyGetter())
{
return getProperty.GetUlong("FreePhysicalMemory", "Win32_OperatingSystem");
}
}

///
/// Number, in megabytes, of virtual memory.
///
public ulong TotalVirtualMemorySize()
{
using (PropertyGetter getProperty = new PropertyGetter())
{
return getProperty.GetUlong("TotalVirtualMemorySize", "Win32_OperatingSystem");
}
}

///
/// Number, in megabytes, of virtual memory currently unused and available.
///
public ulong FreeVirtualMemory()
{
using (PropertyGetter getProperty = new PropertyGetter())
{
return getProperty.GetUlong("FreeVirtualMemory", "Win32_OperatingSystem");
}
}
}

///
/// The Win32_ComputerSystem WMI class represents a computer system running Windows.
///
public class Win32ComputerSystem
{

public Win32ComputerSystem() { }

///
/// Key of a CIM_System instance in an enterprise environment.
/// This property is inherited from CIM_ManagedSystemElement.
///
public string Name()
{
using (PropertyGetter getProperty = new PropertyGetter())
{
return getProperty.GetString("Name", "Win32_ComputerSystem");
}
}

///
/// Name of a computer manufacturer.
///
public string Manufacturer()
{
using (PropertyGetter getProperty = new PropertyGetter())
{
return getProperty.GetString("Manufacturer", "Win32_ComputerSystem");
}
}

///
/// Product name t

Your PropertyGetter.Dispose method does nothing and has nothing to do, so there's no point in PropertyGetter being disposable.

Instead its methods could be static, for example:

public static ulong GetUlong(string propertyName, string Win32Class) { ... }

You can call static methods without using a new PropertyGetter for example:

public ulong FreePhysicalMemory()
{
    return PropertyGetter.GetUlong("FreePhysicalMemory", "Win32_OperatingSystem");
}

By the way, you should theoretically call Dispose on the result of Get() as well as on the result of GetEnumerator():
because the result of Get() is a ManagementObjectCollection instance which is IDisposable, and the result of GetEnumerator() is ManagementObjectEnumerator which is also IDisposable (as well as IEnumerable).

So instead of this ...

using (var enu = moSearcher.Get().GetEnumerator())
{
    ... etc ...
}

... this instead ...

using (var collection = moSearcher.Get())
{
    using (var enu = collection.GetEnumerator())
    {
        ... etc ...
    }
}

However there are members of my group that have an aversion to static classes as they fear they lead to things staying in memory longer that a manually instantiated and disposed class. Can you help shed some light on the subject?

(The following example pseudo-code won't compile, because it uses a too-simplistic version of the SqlConnection API; but I hope it illustrates the point, i.e. object lifetimes.)

Static data exists forever (which may be bad):

static class Foo
{
    // this SqlConnection exists forever!
    static SqlConnection connection = new SqlConnection("sql connection string");

    public static string selectUserName(int userId)
    {
        return connection("select username from users where userid=" + userid.ToString());
    }
}

Local (non-static) data in a static method is eligible for garbage collection as soon as the static method returns (which may be harmless):

static class Foo
{    
    // No static data here!

    public static string selectUserName(int userId)
    {
        // this SqlConnection is temporary
        using (SqlConnection connection = new SqlConnection("sql connection string"))
        {
            return connection("select username from users where userid=" + userid.ToString());
        }
    }
}

The above class has no instance data so there's no reason to construct it: it can be a static class.

Alternatively it could be coded with instance data:

// implements IDisposable because it contains a data member which needs to be disposed
class Foo : IDisposable
{
    // this non-static SqlConnection exists for the same lifetime as
    // the Foo instance[s] which contain[s] it
    SqlConnection connection;

    public Foo()
    {
        SqlConnection connection = new SqlConnection("sql connection string");
    }

    // can't be static because it uses non-static 'connection' member
    public string selectUserName(int userId)
    {
        return connection("select username from users where userid=" + userid.ToString());
    }

    public void Dispose()
    {
        connection.Dispose();
    }
}

The disadvantage of this last one is:

• You must instantiate a Foo because you call a non-static method

• You should remember to dispose the Foo perhaps with a using statement

The advantage of this last one is:

• You can construct a Foo and then call selectUserName several times, using the same Foo instance (therefore the same SqlConnection instance) each time (which may be good if SqlConnection is expensive to construct)

• You can Dispose Foo (and therefore Dispose the SqlConnection instance) when you have finished with it (which may be better than a static SqlConnection instance which exists forever)

In summary the members of your group are semi-correct:

• Static data exists forever (e.g. instance member data of a static object exist forever)

• Local variables in a static method don't exist forever

The code you posted in the OP only uses local data (not instance data, and not static data); therefore it would be better as static methods in a static class.

StackExchange Code Review Q#43598, answer score: 7