HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Single instancing class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
singleclassinstancing

Problem

I have created the following class that is used to provide single instancing functionality to an application. It is called like:

if (Helpers.SingleInstance.IsAlreadyRunning())
{
    Helpers.SingleInstance.ShowRunningApp();
    return;
}


The idea of the class is to prevent the program from being ran twice and it does this by using the applications Guid, as it is a C# program these should always be present. It also allows for extra functionality so a user can override the Guid, this is useful if the same program has different modes it can be ran in so they can both run at the same time.

Lastly it provides the functionality to show the running app, either from the Guid, or from strings contained in the applications main window title.

Here's the class:

```
using System;
using System.Diagnostics;
using System.Drawing;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Security.AccessControl;
using System.Security.Principal;
using System.Threading;
using System.Windows.Forms;

namespace Helpers
{
using static User32;

///
/// A class that provides functionality for single instancing an application.
///
public static class SingleInstance
{
#region Constants

private const uint WINDOW_FLASH_COUNT = 6;
private const uint WINDOW_FLASH_RATE = 70;

#endregion

#region Fields

private static string __appGuid;

private static Mutex _mutex;

#endregion

#region PublicProperties

///
/// If true the application checks if it is running only against the current user, else it checks against all users.
///
public static bool ThisUserOnly { get; set; } = true;

#endregion

#region PrivateProperties

private static string AppGuid
{
get
{
return __appGuid;
}
set
{
__appGuid = value

Solution

I've read through this several times, and I see a couple things that bother me.

First off, this class name just doesn't make sense. I hate to say it, but it really doesn't. When I see class SingleInstance I immediately think Singleton which is not what this is, this is an InstanceManager.

So now that we've named it InstanceManager (or something similar, that was the first name that came to mind that described what it did) we need to look at the API a bit, because some of that rubs me the wrong way as well.

if (Helpers.InstanceManager.IsAlreadyRunning())
{
    Helpers.InstanceManager.ShowRunningApp();
    return;
}


Technically IsAlreadyRunning is not checking for this app, but another app of the same type. So it should be IsInstanceRunning(), then the verbiage makes a little more sense.

Once we clear that up, there's an obvious enhancement that could be made to the API:

public static void ShowIfInstanceRunning()
{
    if (IsInstanceRunning())
    {
        ShowRunningApp();
    }
}


This means in cases where we don't need to return or anything if the other is running we can simply do:

Helpers.InstanceManager.ShowIfInstanceRunning()


Looking more into the API I really don't like the idea of having the ThisUserOnly (which should be CurrentUserOnly) be a property, as well as the AppGuid and requiring an OverrideGuid method. That's just ugly.

Instead, you could supply a Guid? parameter to various methods, if guid.HasValue == false then use the AppGuid, if guid.HasValue == true then use guid.Value. This eliminates the need for OverrideGuid, which is a very ugly API.

If you make these changes then this:

public static void OverrideAppGuid(string text)
{
    using (System.Security.Cryptography.MD5 md5 = System.Security.Cryptography.MD5.Create())
    {
        OverrideAppGuid(new Guid(md5.ComputeHash(System.Text.Encoding.Default.GetBytes(text))));
    }
}


Becomes:

public static Guid GenerateHashedGuid(string text)
{
    using (System.Security.Cryptography.MD5 md5 = System.Security.Cryptography.MD5.Create())
    {
        return new Guid(md5.ComputeHash(System.Text.Encoding.Default.GetBytes(text)));
    }
}


Just as well, ThisUserOnly should be a parameter bool currentUserOnly = true on the appropriate methods.

If you don't want to switch those properties out for parameters, then you should make this an instance class. I see absolutely zero work inside it that could not be done in an instantiated class. Then you can define a constructor InstanceManager(Guid) that you can attach directly to the appropriate application so that if a user wants to check for multiple applications running, it's as simple as:

var thisInstanceManager = new InstanceManager();
var xInstanceManager = new InstanceManager(xGuid);

Code Snippets

if (Helpers.InstanceManager.IsAlreadyRunning())
{
    Helpers.InstanceManager.ShowRunningApp();
    return;
}
public static void ShowIfInstanceRunning()
{
    if (IsInstanceRunning())
    {
        ShowRunningApp();
    }
}
Helpers.InstanceManager.ShowIfInstanceRunning()
public static void OverrideAppGuid(string text)
{
    using (System.Security.Cryptography.MD5 md5 = System.Security.Cryptography.MD5.Create())
    {
        OverrideAppGuid(new Guid(md5.ComputeHash(System.Text.Encoding.Default.GetBytes(text))));
    }
}
public static Guid GenerateHashedGuid(string text)
{
    using (System.Security.Cryptography.MD5 md5 = System.Security.Cryptography.MD5.Create())
    {
        return new Guid(md5.ComputeHash(System.Text.Encoding.Default.GetBytes(text)));
    }
}

Context

StackExchange Code Review Q#154816, answer score: 12

Revisions (0)

No revisions yet.