c++ – ds4mac – directory switcher for macOS/Linux: the tag engine

You can initialize a nested std::vector with the right size in one go like so:

std::vector<std::vector<size_t>> distanceMatrix(len1, std::vector<size_t>(len2));

Since C++17 you can even omit the template parameter for the outer std::vector:

std::vector distanceMatrix(len1, std::vector<size_t>(len2));

However, nested vectors are not great for performance, it is better to have a flat vector with len1 * len2 elements.

You have declared:

friend void operator>>(DirectoryTagEntryList const&, std::ofstream&);

But the normal way is to overload the left shift operator instead, and have the arguments reversed. Furthermore, these operators should take an std::ostream reference, not std::ofstream, so they are even more generic. Finally, these operators should not be void but return the std::ostream reference. So the declaration should instead look like:

friend std::ostream& operator<<(std::ostream&, DirectoryTagEntryList const&);

The same goes for the friend operator>>(std::ifstream&, ...) declarations.

There is also a member function operator<<(std::ifstream& ifs) which doesn’t do anything, and should be removed.

It makes no sense to declare the return value of a function const, unless you are returning a pointer or reference. So the first const in the declaration of DirectoryTagEntryList::size() can be removed.

I see that DirectoryTagEntryList has declared member functions listTags() and listTagsAndDirectories(), but there are no implementations of these functions. I see you have static functions in main.cpp that do the printing, which makes me think you should just remove the useless declarations of the member functions.

In getFilePath(), you do some string manipulation using C string functions before finally creating a std::string of it. You created a memory leak while doing so. I suggest that you convert C strings to std::string as early as possible, and then do all the string manipulation using C++ functions:

static string getTagFilePath() {
    std::string home_directory = getpwuid(getuid())->pw_dir;
    return home_directory + RELATIVE_TAG_FILE_PATH;
}

However, also note that getpwuid() might fail and return NULL. Note that you should check getenv("HOME") first, as is mentioned in the manpage of getpwuid().

I see you are compiling your code with std=c++17. In that case, use C++17’s filesystem library instead of C functions. For example, getting the current directory can be done with std::filesystem::current_path() instead of getcwd(). You can also consider storing pathnames in std::fileystem::paths instead of std::strings.

I see you only check for errors right after opening a file for reading or writing. However, I/O errors can happen at any time. Luckily, you don’t have to error check every I/O operation, instead you can just check the state of the stream right before closing it.

For reading, check at the end that ifs.eof() is true. If not, something obviously went wrong before reaching the end of the file.
For writing, after calling ofs.close(), check that ofs.fail() is false.

You could even consider not checking the state of the stream right after opening it.

Instead of:

for (size_t index = 0, sz = directoryTagEntryList.size(); index < sz; index++) {
    cout << directoryTagEntryList.at(index).getTagName() << "n";
}

It would be much nicer if you could just write:

for (auto &entry: directoryTagEntryList) {
    cout << entry.getTagName() << "n";
}

You can do this by adding begin() and end() member functions to class DirectoryTagEntryList, like so:

class DirectoryTagEntryList {
   std::vector<DirectoryTagEntry> entries;

public:
   auto begin() const {
       return entries.begin();
   }

   auto end() const {
       return entries.end();
   }
   ...
};

Use an existing exception type or create one yourself that inherits an existing type. There’s also no need to catch exceptions if you are going to print them and exit with a non-zero exit code anyway, the same thing will be done automatically if you don’t catch them. So:

int main(...) {
    if (argc == 1) {
        ...
    } else if (argc == 2) {
        ...
    } else if (argc == 3) {
        ...
        throw std::invalid_argument("Flag " + flag + " not recognized");
        ...
    }
}

Also note that you don’t need the explicit return EXIT_SUCCESS at the end of main() in C++.

If you want to avoid the program aborting and potentially causing a core dump for minor errors, then catching the exceptions makes since. But I would then add the try before the body of main(), like so:

int main(...) try {
    // as above
    ...
} catch (const std::exception &ex) {
    std::cerr << ex.what() << "n";
    return EXIT_FAILURE;
}

Instead of parsing options yourself, I recommend you just use getopt_long(). It will ensure parsing is done exactly the same way as most UNIX tools. Unfortunately, it’s not standard, but it is included in the standard library of most operating systems, the most notable exception being Windows. There are also many C and C++ libraries available that provide you with equivalent functionality.