7
\$\begingroup\$

This was a small project I did the other day because I wanted to explore a solution to hot-reload some files in another project.

My requirements were:

  • Act in it's own thread
  • Be able to watch specific file extensions
  • Work on Windows

So since I knew my application would only run on windows I decided to use WinApi.

I am mostly concerned about modern C++ stuff and particularly in handling the thread but if there are any other glaring issues obviously any help is appreciated. I know the FileAction class enum might be a little strange but it just makes the switch statement prettier, not sure if it's frowned upon.

Last note before code is the reason I deleted the copy constructors is because std::thread is not copyable (afaik) but it can be moved so it didn't make sense to allow a copy of the DirectoryWatcherWin class if I couldn't copy all of it's members - hope that's correct..

DirectoryWatcherWin.h

#pragma once
#include <atomic>
#include <thread>
#include <string>
#include <vector>

namespace mikand
{
    enum class FileAction {
        CREATED = FILE_ACTION_ADDED,
        REMOVED = FILE_ACTION_REMOVED,
        MODIFIED = FILE_ACTION_MODIFIED
    };

    class DirectoryWatcherWin
    {
    public:
        using DWWCallback = std::function<void(std::string, FileAction)>;
    private:
        HANDLE m_DirectoryHandle;
        std::atomic<bool> m_Running;
        std::thread m_Thread;
        std::string m_DirectoryToWatch;
        std::vector<std::string> m_Extensions;
        DWWCallback m_Callback;
    public:
        DirectoryWatcherWin(const std::string& directoryToWatch, DWWCallback callback);
        DirectoryWatcherWin(const std::string& directoryToWatch, const std::vector<std::string>& extensionsToWatch, DWWCallback callback);

        DirectoryWatcherWin(const DirectoryWatcherWin& other) = delete;
        DirectoryWatcherWin& operator=(const DirectoryWatcherWin& other) = delete;

        DirectoryWatcherWin(DirectoryWatcherWin&& other) = default;
        DirectoryWatcherWin& operator=(DirectoryWatcherWin&& other) = default;

        ~DirectoryWatcherWin();

        void start();
        void stop();
    private:
        void watch();
    };
}

DirectoryWatcherWin.cpp

#include "pch.h"
#include "DirectoryWatcherWin.h"
#include "StringUtil.h"

namespace mikand
{
    DirectoryWatcherWin::DirectoryWatcherWin(const std::string& directoryToWatch, DWWCallback callback)
        : m_DirectoryToWatch(directoryToWatch), m_Callback(callback)
    {

    }

    DirectoryWatcherWin::DirectoryWatcherWin(const std::string& directoryToWatch, const std::vector<std::string>& extensionsToWatch, DWWCallback callback)
        : m_DirectoryToWatch(directoryToWatch), m_Extensions(extensionsToWatch), m_Callback(callback)
    {

    }

    DirectoryWatcherWin::~DirectoryWatcherWin()
    {
        if (m_Thread.joinable())
        {
            stop();
        }
    }

    void DirectoryWatcherWin::start()
    {
        std::wstring dirWStr = std::wstring(m_DirectoryToWatch.begin(), m_DirectoryToWatch.end());
        m_DirectoryHandle = CreateFile(dirWStr.c_str(),
            GENERIC_READ,
            FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
            NULL,
            OPEN_EXISTING,
            FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED,
            NULL);
        m_Running = true;
        m_Thread = std::thread(&DirectoryWatcherWin::watch, this);
    }

    void DirectoryWatcherWin::stop()
    {
        m_Running = false;
        CancelIoEx(m_DirectoryHandle, 0);
        m_Thread.join();
        CloseHandle(m_DirectoryHandle);
    }

    void DirectoryWatcherWin::watch()
    {
        while (m_Running.load())
        {
            std::vector<BYTE> byteBuffer(4096);
            DWORD bytesReturned;
            OVERLAPPED overlapped = { 0 };
            overlapped.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);

            ReadDirectoryChangesW(m_DirectoryHandle,
                &byteBuffer[0],
                byteBuffer.size(),
                TRUE,
                FILE_NOTIFY_CHANGE_LAST_WRITE | FILE_NOTIFY_CHANGE_CREATION | FILE_NOTIFY_CHANGE_FILE_NAME,
                &bytesReturned,
                &overlapped,
                NULL);

            GetOverlappedResult(m_DirectoryHandle, &overlapped, &bytesReturned, TRUE);

            FILE_NOTIFY_INFORMATION *fni = (FILE_NOTIFY_INFORMATION*)(&byteBuffer[0]);
            do
            {
                std::string filenameStr = StringUtil::wcharToString(fni->FileName, fni->FileNameLength);
                if (m_Extensions.size() > 0)
                {
                    for (auto& extension : m_Extensions)
                    {
                        if (StringUtil::endsWith(filenameStr, extension))
                        {
                            m_Callback(filenameStr, FileAction(fni->Action));
                            break;
                        }
                    }
                }
                else
                {
                    m_Callback(filenameStr, FileAction(fni->Action));
                }
                fni = (FILE_NOTIFY_INFORMATION*)((BYTE*)(fni) + fni->NextEntryOffset);
            } while (fni->NextEntryOffset != 0);
        }
    }
}

StringUtil.h

namespace mikand
{
    class StringUtil
    {
    public:

        inline static bool endsWith(const std::string& str, const std::string& suffix)
        {
            return (str.length() >= suffix.length()) && (str.compare(str.length() - suffix.length(), suffix.length(), suffix) == 0);
        }

        inline static bool startsWith(const std::string& str, const std::string& prefix)
        {
            return (str.length() >= prefix.length()) && (str.compare(0, prefix.size(), prefix) == 0);
        }

        inline static std::string wcharToString(wchar_t* in, unsigned long length)
        {
            if (length < 1)
            {
                return std::string();
            }
            std::wstring_convert<std::codecvt_utf8<wchar_t>, wchar_t> converter;
            std::wstring tempWstr(in, length);
            std::string s = converter.to_bytes(tempWstr);
            int i = s.length() - 1;
            while (s.at(i) == '\0')
            {
                s.pop_back();
                i--;
            }
            return s;
        }
// UNUSED METHODS OMITTED
    };
}

pch.h

#ifndef PCH_H
#define PCH_H

#include <windows.h>
#include <string>
#include <vector>
#include <array>
#include <locale>
#include <codecvt>
#include <iostream>


#endif

main.cpp

#include "pch.h"
#include "DirectoryWatcherWin.h"

using namespace mikand;

void printFilename(std::string filename, FileAction action)
{
    switch (action)
    {
    case FileAction::CREATED:
        std::cout << "Created: " << filename << std::endl;
        break;
    case FileAction::REMOVED:
        std::cout << "Removed: " << filename << std::endl;
        break;
    case FileAction::MODIFIED:
        std::cout << "Modified: " << filename << std::endl;
        break;
    default:
        break;
    }
}

int main()
{
    DirectoryWatcherWin::DWWCallback callback = printFilename;
    DirectoryWatcherWin watcher("C:\\temp", std::vector<std::string>{".txt"}, callback);

    watcher.start();

    std::string waitForPress;
    std::getline(std::cin, waitForPress);

    watcher.stop();

    system("pause");
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Style

  • Subjective but don't indent on namespaces.

  • I would group the ctors and operators together instead of grouping them by their status (default/delete).

  • Your ctor is a bit hard to read. Consider formatting it differently. E.g.:

    DirectoryWatcherWin::DirectoryWatcherWin(
            const std::string& directoryToWatch, 
            const std::vector<std::string>& extensionsToWatch, 
            DWWCallback callback)
        : m_DirectoryToWatch(directoryToWatch)
        , m_Extensions(extensionsToWatch)
        , m_Callback(callback)
    {}    
    

    This way the parameters are visually separated from the member init list and you don't break any line length recommendations either.


Code

  • We need to talk about your interface.
    Generally you should start with public and private should come last. Also do not use those keywords more than once per class definition.
    I would probably either ditch the DWWCallback declaration, or move it into the namespace, seeing as there is probably no use-case where people use your namespace but not the DWWCallback.

  • Prefer prefix over postfix.

  • Prefer using \n over std::endl.

  • Don't using namespace. Prefixing your code with the namespace is really not too much to ask.

  • for (auto& extension : m_Extensions)
    

    Consider using const in loops like this if you don't intend to modify the loop variable in the body.

  • If you have strings that you don't modify consider using string_view as it can be faster.

  • When you want to initialize a struct and all its members consider using foostruct = {}; See this for more info.

  • std::wstring dirWStr = std::wstring(m_DirectoryToWatch.begin(), m_DirectoryToWatch.end());
    

    I don't quite follow the idea behind this. Can you not pass this directly as something else like maybe mystring_view.data()? If it has to be a wstr why not keep it as such in the class as well?

\$\endgroup\$
3
  • 3
    \$\begingroup\$ The "pch.h" header is a Microsoft thing, replacing "stdafx.h". Not shown here, but included in the project, is a "pch.cpp" file whose sole purpose in life is to generate the precompiled header that will be read in by all the other source files. \$\endgroup\$ Jun 6, 2019 at 20:33
  • \$\begingroup\$ @1201ProgramAlarm I see, thanks for the clarification. \$\endgroup\$
    – yuri
    Jun 7, 2019 at 6:19
  • \$\begingroup\$ Thanks for the answer! First point, I've just always structured it as public variables, private variables, then public functions, private functions but I see your point. The "using namespace" was just in the sample to save some typing. More interesting I think though is the last point: the reason is it has to be a wstring for WinAPI to be happy but to save other people from using wstring I provided an std::string interface instead. The conversion has to happen somewhere and I thought doing it on the "inside" would be a bit neater. \$\endgroup\$
    – Mikael
    Jun 8, 2019 at 19:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.