A C++ WinAPI program for changing the process priority classes via PIDs - take 2

Question

After improving the previous post, I came up with the following program:

#include <Windows.h>
#include <iostream>
#include <sstream>
#include <string>

using std::wcout;
using std::wcerr;
using std::wstring;
using std::wstringstream;

static const DWORD BAD_PRIORITY_CLASS_SELECTION = 0xffff0000;

static void MyPrintHelp();
static bool MyAttemptToChangeProcessPriorityClass(DWORD pid, 
                                                  DWORD priorityClassFlag);

static bool MyCheckPriorityClassSelection(DWORD priorityClassSelection);
static DWORD MyConvertPriorityClassSelectionToFlag(DWORD priorityClassSelection);
static wstring MyConvertProcessClassFlagToString(DWORD priorityClassFlag);

int wmain(int argc, const wchar_t** args) {
    if (argc != 3) {
        ::MyPrintHelp();
        return 0;
    }

    DWORD pid;
    DWORD priorityClassFlag;
    DWORD priorityClassSelection;

    // Read the PID and the process priority class selection:
    wstringstream wss;
    wss << args[1] << L" " << args[2];
    wss >> pid;

    if (wss.fail() || wss.eof()) {
        wcerr << L"Error: bad PID (" << args[1] << L"\n";
        return EXIT_FAILURE;
    }

    wss >> priorityClassSelection;

    if (wss.fail()) {
        wcerr << L"Error: bad priority class selector (" << args[2] << L")\n";
        return EXIT_FAILURE;
    }

    // Once here, reading and parsing the PID and the priority class succeeded:
    if (!::MyCheckPriorityClassSelection(priorityClassSelection)) {
        // Here, the priority class selection is out of bounds:
        wcerr << L"Error: bad priority class selection: " 
              << priorityClassSelection
              << L". Must be between 0 and 5, inclusively.\n";

        return EXIT_FAILURE;
    }

    // Get the WinAPI flag for the selected priority class:
    priorityClassFlag = 
        ::MyConvertPriorityClassSelectionToFlag(priorityClassSelection);

    if (priorityClassFlag == BAD_PRIORITY_CLASS_SELECTION) {
        wcerr << L"Error: unknown priority class selection: "
              << priorityClassSelection
              << L"\n";

        return EXIT_FAILURE;
    }

    return ::MyAttemptToChangeProcessPriorityClass(pid, priorityClassFlag) ? 
        EXIT_SUCCESS :
        EXIT_FAILURE;
}

static void MyPrintHelp() {
    wcout << L"prioset.exe PID PRIORITY_CLASS\n"
          << L"Where PRIORITY_CLASS is one of:\n"
          << L"    0 - IDLE_PRIORITY_CLASS\n"
          << L"    1 - BELOW_NORMAL_PRIORITY_CLASS\n"
          << L"    2 - NORMAL_PRIORITY_CLASS\n"
          << L"    3 - ABOVE_NORMAL_PRIORITY_CLASS\n"
          << L"    4 - HIGH_PRIORITY_CLASS\n"
          << L"    5 - REALTIME_PRIORITY_CLASS\n"
          << L"    REALTIME_PRIORITY_CLASS requires administrator "
          << L"privileges.\n";
}

static wstring GetLastErrorAsString(DWORD errorMessageId)
{
    LPWSTR messageBuffer = nullptr;

    size_t size = ::FormatMessage(
                        FORMAT_MESSAGE_ALLOCATE_BUFFER | 
                        FORMAT_MESSAGE_FROM_SYSTEM | 
                        FORMAT_MESSAGE_IGNORE_INSERTS,
        NULL, 
        errorMessageId, 
        MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), 
        (LPWSTR) &messageBuffer, 0, NULL);

    // Copy the error message into a std::wstring:
    wstring message(messageBuffer, size);

    // Free the Win32's string's buffer:
    ::LocalFree(messageBuffer);
    return message;
}

static bool MyAttemptToChangeProcessPriorityClass(DWORD pid, DWORD priorityClassFlag) {
    HANDLE processHandle = 
        ::OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_SET_INFORMATION, 
            false, 
            pid);

    if (processHandle == NULL) {
        wcout << L"Error: could not open a process with PID " << pid << "\n";
        return false;
    }

    DWORD oldPriorityClass = ::GetPriorityClass(processHandle);

    if (oldPriorityClass == 0) {
        DWORD errorCode = ::GetLastError();
        wcerr << L"Error: " 
              << ::GetLastErrorAsString(errorCode) 
              << L"Error code: " 
              << errorCode 
              << L"\n";

        ::CloseHandle(processHandle);
        return false;
    }

    if (!::SetPriorityClass(processHandle, priorityClassFlag)) {
        DWORD errorCode = ::GetLastError();
        wcerr << L"Error: "
            << ::GetLastErrorAsString(errorCode)
            << L"Error code: "
            << errorCode
            << L"\n";

        ::CloseHandle(processHandle);
        return false;
    }

    DWORD newPriorityClass = ::GetPriorityClass(processHandle);

    if (newPriorityClass == 0) {
        DWORD errorCode = ::GetLastError();
        wcerr << L"Error: "
            << ::GetLastErrorAsString(errorCode)
            << L"Error code: "
            << errorCode
            << L"\n";

        ::CloseHandle(processHandle);
        return false;
    }

    ::CloseHandle(processHandle);

    if (newPriorityClass == oldPriorityClass) {
        wcout << L"The requested priority class is the same "
              << L"as the current priority class ("
              << ::MyConvertProcessClassFlagToString(newPriorityClass)
              << L").\n";

        return true;
    }

    if (newPriorityClass == priorityClassFlag) {
        wcout << L"Successfully changed the priority class from "
              << ::MyConvertProcessClassFlagToString(oldPriorityClass)
              << L" ("
              << oldPriorityClass
              << L") to "
              << ::MyConvertProcessClassFlagToString(newPriorityClass)
              << L" ("
              << newPriorityClass
              << L")\n";
    } else {
        wcout << L"Warning: Could not change the priority class from "
              << ::MyConvertProcessClassFlagToString(oldPriorityClass)
              << L" ("
              << oldPriorityClass
              << L") to "
              << ::MyConvertProcessClassFlagToString(priorityClassFlag)
              << L" ("
              << priorityClassFlag
              << L"). Instead, "
              << ::MyConvertProcessClassFlagToString(newPriorityClass)
              << L" ("
              << newPriorityClass
              << L") is used as a new priority class.\n";
    }

    return true;
}

static bool MyCheckPriorityClassSelection(DWORD priorityClassSelection) {
    return priorityClassSelection >= 0 || priorityClassSelection < 6;
}

static DWORD MyConvertPriorityClassSelectionToFlag(
    DWORD priorityClassSelection) {
    switch (priorityClassSelection) {
        case 0:
            return IDLE_PRIORITY_CLASS;

         case 1:
            return BELOW_NORMAL_PRIORITY_CLASS;

         case 2:
            return NORMAL_PRIORITY_CLASS;

         case 3:
            return ABOVE_NORMAL_PRIORITY_CLASS;

         case 4:
            return HIGH_PRIORITY_CLASS;

         case 5:
            return REALTIME_PRIORITY_CLASS;

         default:
            return BAD_PRIORITY_CLASS_SELECTION;
    }
}

static wstring MyConvertProcessClassFlagToString(DWORD priorityClassFlag) {
    switch (priorityClassFlag) {
    case IDLE_PRIORITY_CLASS:
        return L"IDLE_PRIORITY_CLASS";

    case BELOW_NORMAL_PRIORITY_CLASS:
        return L"BELOW_NORMAL_PRIORITY_CLASS";

    case NORMAL_PRIORITY_CLASS:
        return L"NORMAL_PRIORITY_CLASS";

    case ABOVE_NORMAL_PRIORITY_CLASS:
        return L"ABOVE_NORMAL_PRIORITY_CLASS";

    case HIGH_PRIORITY_CLASS:
        return L"HIGH_PRIORITY_CLASS";

    case REALTIME_PRIORITY_CLASS:
        return L"REALTIME_PRIORITY_CLASS";

    default:
        return L"BAD_PRIORITY_CLASS";
    }
}

Critique request

Please tell me anything that comes to mind.

Answer 1

General Observations

I think that I would have created most of this code as a class so that it could be reused by other programs. The code would also be better if it followed the SOLID programming principles. SOLID is 5 object oriented design principles. SOLID is a mnemonic acronym for five design principles intended to make software designs more understandable, flexible and maintainable. This will help you design your objects and classes better.

1. The Single Responsibility Principle - A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.
2. The Open–closed Principle - states software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.
3. The Liskov Substitution Principle - Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program.
4. The Interface segregation principle - states that no client should be forced to depend on methods it does not use.
5. The Dependency Inversion Principle - is a specific form of decoupling software modules. When following this principle, the conventional dependency relationships established from high-level, policy-setting modules to low-level, dependency modules are reversed, thus rendering high-level modules independent of the low-level module implementation details.

Code Organization

Function prototypes are very useful in large programs that contain multiple source files, and that in case they will be in header files. In a single file program like this it is better to put the main() function at the bottom of the file and all the functions that get used in the proper order above main(). Keep in mind that every line of code written is another line of code where a bug can crawl into the code.

Performance and Maintainability

Switch/case statements are harder to maintain for conversion functions then some other implementations of conversion. The fastest form of conversion is to use arrays (presented as old style C arrays but since this is C++, C++ container class arrays could also be used). By using arrays we remove one magic number (6), and the code is reduced in size. Maintenance is then limited to adding new constants to the arrays. This also allows the conversion to all be in terms of the user input, rather than converting from user input to a priority class and then converting the priority class to strings if necessary. The conversion functions could also be in a class of their own.

static DWORD priorityConversion[] =
{
    IDLE_PRIORITY_CLASS,
    BELOW_NORMAL_PRIORITY_CLASS,
    NORMAL_PRIORITY_CLASS,
    ABOVE_NORMAL_PRIORITY_CLASS,
    HIGH_PRIORITY_CLASS,
    REALTIME_PRIORITY_CLASS
};

static wstring priortyWStringConverter[] =
{
    L"IDLE_PRIORITY_CLASS",
    L"BELOW_NORMAL_PRIORITY_CLASS",
    L"NORMAL_PRIORITY_CLASS",
    L"ABOVE_NORMAL_PRIORITY_CLASS",
    L"HIGH_PRIORITY_CLASS",
    L"REALTIME_PRIORITY_CLASS"
};

static const size_t MaxPriorityClass = sizeof(priorityConversion) /
    sizeof(*priorityConversion);

static bool MyCheckPriorityClassSelection(DWORD priorityClassSelection) {
    return priorityClassSelection >= 0 || priorityClassSelection < MaxPriorityClass;
}

static DWORD MyConvertPriorityClassSelectionToFlag(DWORD priorityClassSelection)
{
    if (!MyCheckPriorityClassSelection(priorityClassSelection))
    {
        return BAD_PRIORITY_CLASS_SELECTION;
    }
    else
    {
        return priorityConversion[priorityClassSelection];
    }
}

static wstring MyConvertProcessClassFlagToString(DWORD priorityClassSelection)
{
    if (!MyCheckPriorityClassSelection(priorityClassSelection))
    {
        return L"BAD_PRIORITY_CLASS";
    }
    else
    {
        return priortyWStringConverter[priorityClassSelection];
    }

}

Complexity

Two of the functions, wmain(int argc, const wchar_t** args) and MyAttemptToChangeProcessPriorityClass(DWORD pid, DWORD priorityClassFlag) are too complex (they do too much, and the functions are too long). Both would benefit by creating smaller simpler functions that follow the Single Responsibility Principle and then calling those smaller functions in the larger functions. I suggest moving the error handling to the smaller functions, some examples are:

static HANDLE getProcessHandle(DWORD pid)
{
    HANDLE processHandle =
        ::OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_SET_INFORMATION, false, pid);

    if (processHandle == NULL) {
        wcout << L"Error: could not open a process with PID " << pid << "\n";
    }

    return processHandle;
}

static DWORD getAndCheckOldPriority(HANDLE processHandle)
{
    DWORD oldPriorityClass = ::GetPriorityClass(processHandle);

    if (oldPriorityClass == 0) {
        DWORD errorCode = ::GetLastError();
        wcerr << L"Error: "
            << ::GetLastErrorAsString(errorCode)
            << L"Error code: "
            << errorCode
            << L"\n";

        ::CloseHandle(processHandle);
    }

    return oldPriorityClass;
}

static bool MyAttemptToChangeProcessPriorityClass(DWORD pid, DWORD priorityClassFlag)
{
    HANDLE processHandle = getProcessHandle(pid);
    if (processHandle == NULL) {
        return false;
    }

    DWORD oldPriorityClass = getAndCheckOldPriority(processHandle);
    if (oldPriorityClass == 0) {
        return false;
    }

    ...
}

Answer 2

I'll comment on the argument parsing.

Your code inserts both arguments into the same string stream and then extracts the integer values from that string. As a consequence, not all invalid input is detected. Here are some examples:

prioset.exe 123x 456      // Error: bad PID
prioset.exe 123 456x      // No error, PID = 123, PRIORITY = 456
prioset.exe "123 456" xxx // No error, PID = 123, PRIORITY = 456

It is better to parse the arguments separately, and check that the complete string has been parsed:

wstringstream wss (args[1]);
wss >> pid;
if (wss.fail() || !wss.eof()) {
    // Report error
    return EXIT_FAILURE;
}

Then do the same for the second argument. You may want to move that into a separate function in order to avoid code duplication.

For more ways to parse an integer from a string, see for example this overview on Stack Overflow. Depending on the supported C++ version, you can use more sophisticated functions like std::from_chars.