Single Responsibility - is this class doing too much?

Question

I'm redesigning one of my programs which performs certain actions on processes of interest (known as "Monitored Processes" in my program).

Some actions I always need to do on those processes are:

• Open process handle
• Get process name
• Close process handle

And some further actions I may need to do on some or all of them are:

• Suspend process
• Resume process
• Set process priority
• Set process affinity

Previously I simply had a "handler" class that opened a handle to the process, checked its name, and performed the requested actions. All of that is done via p/invoke, I don't use .NET's Process class at all.

However I am trying to redesign it from a SOLID perspective, so I created a "MonitoredProcess" object and have added a number of methods to it to perform the actions.

Here it is in its current state:

public class MonitoredProcess : IMonitoredProcess
{
    private readonly IMonitoredProcessConfig config;
    private readonly int processId;
    private IntPtr handle = IntPtr.Zero;
    private uint suspendResumeResult;
    private string processName;

    public IMonitoredProcessConfig Config { get { return config; } }
    public int ProcessId { get { return processId; } }
    public uint SuspendResumeResult { get { return suspendResumeResult; } }
    public string ProcessName
    {
        get
        {
            if (processName == null) PopulateProcessName();

            return processName;
        }
    }

    public MonitoredProcess(IMonitoredProcessConfig config, int processId)
    {
        if (config == null) throw new ArgumentNullException("config");

        this.config = config;
        this.processId = processId;
    }


    public bool OpenProcess()
    {
        var alreadyOpen = handle != IntPtr.Zero;
        if (alreadyOpen) return true;

        handle = Kernel32.OpenProcess(config.RequiredRights, false, (uint)processId);
        return handle != IntPtr.Zero;
    }

    public bool CloseProcess()
    {
        var closed = Kernel32.CloseHandle(handle);
        if (closed) handle = IntPtr.Zero;
        return closed;
    }

    public uint Resume()
    {
        suspendResumeResult = Ntdll.NtResumeProcess(handle);
        return suspendResumeResult;
    }

    public uint Suspend()
    {
        suspendResumeResult = Ntdll.NtSuspendProcess(handle);
        return suspendResumeResult;
    }

    private void PopulateProcessName()
    {
        var buffer = new StringBuilder((int)Kernel32.GeneralConstants.MAX_PATH);
        var success = 0 != Psapi.GetModuleFileNameEx(
            handle,
            IntPtr.Zero,
            buffer,
            (uint)buffer.Capacity
            );

        processName = success ? Path.GetFileName(buffer.ToString()) : string.Empty;
    }

}

I haven't yet added methods to set the process's priority and affinity. The interface it implements is not yet really defined, it will become whatever the class eventually exposes.

Question:

Is this class doing too much? Should a "process" know how to suspend/resume itself, and set its own priority and CPU affinity? Or should I have other classes that take an IMonitoredProcess and perform these actions on them (which is how I did it before)?

---

In the way I did it before, the classes performing the actions implemented them through p/invoke, so they "knew" they could throw and catch Win32Exceptions to get the error message. If this MonitoredProcess class implements everything via p/invoke internally, the class calling those methods technically shouldn't know how they're implemented, so has no reason to either throw or catch Win32Exceptions.

And I wouldn't want to simply throw them from this class (even wrapped in a domain-specific exception) because in half the places I call the methods I'm not interested in the outcome, but I'd still have to try/catch it every time - and it creates objects unnecessarily.

Which again makes me wonder if this class should really be doing all these actions, ostensibly on itself, but it uses external functions to perform all of them.

Answer 1

I would call the class ProcessController rather then MonitoredProcess. The class instance is not a process, it encapsulates a process handle. And it does not just monitor, it controls.

You could pass the process id to the constructor.

This would get your terminology straight, it would be clear your class controls a process for you. Which is one responsibility. So you're good.

Answer 2

The most useful way I've found to understand the Single Responsibility Principle is to interpret "responsibility" as "reason why the code might change". This is because the primary goal of SRP is to group together code that will change together, and (as far as possible) keep code that will change for different reasons separated from it. By doing this, we make it easier to make those changes if they're ever required.

In your case, the only reason you'd need to change your code is if the underlying system code that you're calling via p/invoke changes (either because a new version of Windows changes API in future, or if you decide to port your project to a different operating system).

From this point of view, your code is fine, although it may be worth (if you can ever foresee the possibility of an OS change) grouping this code together with any other system-dependent code, either in a subdirectory or in a library project.

The one change I would make is to encapsulate your return values in something more meaningful than a generic uint - as it stands, your clients need to understand the underlying Win32 implementation, which is a violation of the Law of Demeter. Create a specific return type that encapsulates the value returned by Windows.

Answer 3

This depends largely on the context of a "MonitoredProcess" in your application as a whole.

Look at the name you've used: MonitoredProcess.

This implies that the process is being monitored by something. It may be a different class, method, or something else in your application that is monitoring your processes and deciding on whether or not it wants to perform these actions.

Based on that you have to determine if the monitor itself is more concerned with executing actions on processes or tracking results of actions on processes. In the former situation, it would be the monitor that wants to be able to perform the Open(), Close(), Resume(), and Suspend() operation on a process, not the MonitoredProcess that wants to perform these actions on itself. In the latter, the MonitoredProcess would want to contain these actions and the monitor would simply wants to invoke these operations on the process itself and manage the results.