2
\$\begingroup\$

I have this program in GitHub. It is used for switching to directories via custom tags. Here is the tag engine:

com::github::coderodde::ds4mac::DirectoryTagEntry.h:

//// /////////////////////////////////////////////  ////
 // Created by Rodion "rodde" Efremov on 19.11.2020. //
//// ////////////////////////////////////////////// ////
#ifndef COM_GITHUB_CODERODDE_DS4MAC_DIRECTORY_TAG_ENTRY_H
#define COM_GITHUB_CODERODDE_DS4MAC_DIRECTORY_TAG_ENTRY_H

#include <fstream>
#include <iostream>
#include <string>

namespace com::github::coderodde::ds4mac {

    class DirectoryTagEntry {
    private:
        std::string tagName;
        std::string directoryName;

    public:

        DirectoryTagEntry(std::string tagName, std::string directoryName);

        const std::string& getTagName() const;
        const std::string& getDirectoryName() const;

        void setTagName(const std::string& newTagName);
        void setDirectoryName(const std::string& newDirectoryName);

        size_t getLevenshteinDistance(std::string str) const;

        friend void operator>>(std::ifstream& ifs, DirectoryTagEntry& dte) {
            std::string readTagName;
            std::string readDirectoryName;

            std::getline(ifs, readDirectoryName);
            dte.setTagName(readTagName);
            dte.setDirectoryName(readDirectoryName);
        }

        friend void operator<<(std::ofstream& ofs, const DirectoryTagEntry& dte) {
            ofs << dte.getTagName() << " " << dte.getDirectoryName();
        }

        static struct {
            bool operator()(DirectoryTagEntry const& a, DirectoryTagEntry const& b) {
                return a.getTagName() < b.getTagName();
            }
        } tagComparator;

        static struct {
            bool operator()(DirectoryTagEntry const& a, DirectoryTagEntry const& b) {
                return a.getDirectoryName() < b.getDirectoryName();
            }
        } directoryComparator;
    };
}

#endif // COM_GITHUB_CODERODDE_DS4MAC_DIRECTORY_TAG_ENTRY_H

com::github::coderodde::ds4mac::DirectoryTagEntry.cpp:

//// ////////////////////////////////////////////// ////
 // Created by Rodion "rodde" Efremov on 19.11.2020. //
//// ////////////////////////////////////////////// ////

#include "DirectoryTagEntry.h"
#include <utility>
#include <vector>

namespace com::github::coderodde::ds4mac {

    using std::size_t;

    DirectoryTagEntry::DirectoryTagEntry(
        std::string tagName_,
        std::string directoryName_) :
        tagName{std::move(tagName_)},
        directoryName{std::move(directoryName_)} {}

    const std::string& DirectoryTagEntry::getTagName() const {
        return tagName;
    }

    const std::string& DirectoryTagEntry::getDirectoryName() const {
        return directoryName;
    }      

    void DirectoryTagEntry::setTagName(const std::string& newTagName) {
        tagName = newTagName;
    }

    void DirectoryTagEntry::setDirectoryName(const std::string& newDirectoryName) {
        directoryName = newDirectoryName;
    }

    size_t DirectoryTagEntry::getLevenshteinDistance(const std::string str) const {
        const size_t len1 = str.length() + 1;
        const size_t len2 = tagName.length() + 1;

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

        for (size_t i = 0; i < len1; i++) {
            std::vector<size_t> row(len2);
            distanceMatrix[i] = row;
        }

        for (size_t i = 1; i < len1; i++) {
            distanceMatrix[i][0] = i;
        }

        for (size_t i = 1; i < len2; i++) {
            distanceMatrix[0][i] = i;
        }

        for (size_t i1 = 1; i1 < len1; i1++) {
            for (size_t i2 = 1; i2 < len2; i2++) {
                size_t cost = (str[i1 - 1] == tagName[i2 - 1] ? 0 : 1);

                distanceMatrix[i1][i2] =
                       std::min(
                                std::min(distanceMatrix[i1 - 1][i2] + 1,
                                         distanceMatrix[i1][i2 - 1] + 1),
                                         distanceMatrix[i1 - 1][i2 - 1] + cost);
            }
        }      

        return distanceMatrix[len1 - 1][len2 - 1];
    }
}

com::github::coderodde::ds4mac::DirectoryTagEntryList.h:

//// /////////////////////////////////////////////  ////
 // Created by Rodion "rodde" Efremov on 20.11.2020. //
//// ////////////////////////////////////////////// ////

#ifndef COM_GITHUB_CODERODDE_DS4MAC_DIRECTORY_TAG_ENTRY_LIST_H
#define COM_GITHUB_CODERODDE_DS4MAC_DIRECTORY_TAG_ENTRY_LIST_H

#include "DirectoryTagEntry.h"
#include <vector>

namespace com::github::coderodde::ds4mac {

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

    public:
        const size_t size() const;
        DirectoryTagEntryList& operator<<(DirectoryTagEntry const& directoryTagEntry);
        DirectoryTagEntry at(size_t index) const;
        DirectoryTagEntry* operator[](std::string const& targetDirectoryName);
        void operator<<(std::ifstream& ifs);
        void sortByTags();
        void sortByDirectories();
        void listTags();
        void listTagsAndDirectories();
        friend void operator>>(std::ifstream&, DirectoryTagEntryList&);
        friend void operator>>(DirectoryTagEntryList const&, 
                               std::ofstream&);
    };
}

#endif // COM_GITHUB_CODERODDE_DS4MAC_DIRECTORY_TAG_ENTRY_LIST_H

com::github::coderodde::ds4mac::DirectoryTagEntryList.cpp:

//// ////////////////////////////////////////////// ////
 // Created by Rodion "rodde" Efremov on 20.11.2020. //
//// ////////////////////////////////////////////// ////

#include "DirectoryTagEntry.h"
#include "DirectoryTagEntryList.h"
#include <algorithm>
#include <limits>
#include <string>
//#include <linux/limits.h>

using com::github::coderodde::ds4mac::DirectoryTagEntry;
using std::getline;
using std::string;

namespace com::github::coderodde::ds4mac {

    const size_t DirectoryTagEntryList::size() const {
        return entries.size();
    }

    DirectoryTagEntryList& DirectoryTagEntryList::operator<<(
        const DirectoryTagEntry &directoryTagEntry) {
    
        entries.push_back(directoryTagEntry);
        return *this;
    }

    DirectoryTagEntry DirectoryTagEntryList::at(size_t index) const {
        return entries.at(index);
    }

    DirectoryTagEntry* DirectoryTagEntryList::operator[](
        const std::string& targetDirectoryName) {
    
        DirectoryTagEntry* ptrBestDirectoryEntry;
        size_t bestLevenshteinDistance = std::numeric_limits<size_t>::max();

        for (DirectoryTagEntry& dte : entries) {
            size_t levenshteinDistance = dte.getLevenshteinDistance(targetDirectoryName);

            if (levenshteinDistance == 0) {
                return &dte;
            }

            if (bestLevenshteinDistance > levenshteinDistance) {
                bestLevenshteinDistance = levenshteinDistance;
                ptrBestDirectoryEntry = &dte;
            }
        }

        return ptrBestDirectoryEntry;
    }

    void DirectoryTagEntryList::operator<<(std::ifstream& ifs) {

        using std::string;

    }

    void DirectoryTagEntryList::sortByTags() {
        std::stable_sort(entries.begin(), entries.end(), DirectoryTagEntry::tagComparator);
    }

    void DirectoryTagEntryList::sortByDirectories() {
        std::stable_sort(entries.begin(), entries.end(),  DirectoryTagEntry::directoryComparator);
    }

    // trim from start (in place)
    static inline void ltrim(std::string &s) {
        s.erase(s.begin(), std::find_if(s.begin(), s.end(), [](unsigned char ch) {
            return !std::isspace(ch);
        }));
    }

    // trim from end (in place)
    static inline void rtrim(std::string &s) {
        s.erase(std::find_if(s.rbegin(), s.rend(), [](unsigned char ch) {
            return !std::isspace(ch);
        }).base(), s.end());
    }

    // trim from both ends (in place)
    static inline void trim(std::string &s) {
        ltrim(s);
        rtrim(s);
    }

    void operator>>(
        std::ifstream& ifs,
        DirectoryTagEntryList& directoryTagEntryList) {

        while (ifs.good() && !ifs.eof()) {

            string tag;
            ifs >> tag;
            trim(tag);

            // Grab the rest of the line. 
            // We need this isntead of >> in order to obtain 
            // the space characters in the directory names.
            string dir;
            getline(ifs, dir);
            trim(dir);

            DirectoryTagEntry newDirectoryEntry(tag, dir);
            directoryTagEntryList << newDirectoryEntry;
        }
    }

    void operator>>(DirectoryTagEntryList const& directoryTagEntryList,
                    std::ofstream& ofs) {
        for (size_t i = 0, sz = directoryTagEntryList.size();
            i < sz;
            i++) {
            DirectoryTagEntry const& dte = directoryTagEntryList.at(i);
            ofs << dte.getTagName() << " " << dte.getDirectoryName() << "\n";
        }
    }
}

main.cpp:

#include "DirectoryTagEntry.h"
#include "DirectoryTagEntryList.h"
#include <iomanip>
#include <iostream>
#include <cstdlib>
#include <fstream>
#include <string>
#include <cstring>
#include <pwd.h>
#include <unistd.h>
#include <vector>

using com::github::coderodde::ds4mac::DirectoryTagEntry;
using com::github::coderodde::ds4mac::DirectoryTagEntryList;
using std::cerr;
using std::cout;
using std::ifstream;
using std::ofstream;
using std::setw;
using std::string;

//// ///////////////////
 // Operation names: //
/////////////////// ////
const string OPERATION_SWITCH_DIRECTORY = "switch_directory";
const string OPERATION_DESCRIPTOR_SHOW_TAG_ENTRY_LIST =
        "show_tag_entry_list";

//// /////////////////
 // All the flags: //
///////////////// ////
const string FLAG_LIST_TAGS = "-l";
const string FLAG_LIST_BOTH = "-L";
const string FLAG_LIST_TAGS_SORTED = "-s";
const string FLAG_LIST_BOTH_SORTED = "-S";
const string FLAG_LIST_BOTH_SORTED_DIRS = "-d";
const string FLAG_UPDATE_PREVIOUS = "--update-previous";
const string PREV_TAG_NAME = "__dt_previous";

// The path to the tag file, relative to the home directory:
const string RELATIVE_TAG_FILE_PATH = "/.ds/tags";

//// ///////////////////////////////////////////
 // Returns the entire path to the tag file. //
/////////////////////////////////////////// ////
static const string getTagFilePath() {
    char* c_home_directory = getpwuid(getuid())->pw_dir;
    size_t str_len = strlen(c_home_directory) + 1;
    char* c_home_directory_copy = (char*) std::malloc(str_len);
    std::strncpy(c_home_directory_copy,
                 c_home_directory,
                 str_len);

    string path(c_home_directory_copy);
    return path += RELATIVE_TAG_FILE_PATH;
}

//// /////////////////////////////////////////
 // Returns the current working directory: //
///////////////////////////////////////// ////
static string getCurrentWorkingDirectory() {
    char* working_dir = new char[PATH_MAX];
    working_dir = getcwd(working_dir, PATH_MAX);
    string rv = working_dir;
    delete[] working_dir;
    return rv;
}

//// //////////////////////////////////////
 // Returns the maximum length of tags: //
////////////////////////////////////// ////
static const size_t getMaximumTagLength(DirectoryTagEntryList const& dtel) {
    size_t maximumTagLength = 0;

    for (size_t index = 0, sz = dtel.size(); index < sz; index++) {
        maximumTagLength = 
            std::max(maximumTagLength,
                     dtel.at(index).getTagName().length());
    }

    return maximumTagLength;
}

//// //////////////////////////////////
 // Updates the previous directory: //
////////////////////////////////// ////
static string updatePreviousDirectory(
        DirectoryTagEntryList& directoryTagEntryList,
        string& newDirectoryName) {

    DirectoryTagEntry* dte = directoryTagEntryList[PREV_TAG_NAME];

    if (dte->getTagName().compare(PREV_TAG_NAME) == 0) {
        string rv = dte->getDirectoryName();
        dte->setDirectoryName(newDirectoryName);
        return rv;
    } else {
        DirectoryTagEntry ndte(PREV_TAG_NAME, newDirectoryName);
        directoryTagEntryList << ndte;
        return getCurrentWorkingDirectory();
    }
}

//// //////////////////////////////////////////////////
 // Returns the home directory of the current user: //
////////////////////////////////////////////////// ////
static string getHomeDirectory() {
    return string(getpwuid(getuid())->pw_dir);
}

//// //////////////////////////////////////////////////////////////////////
 // Changes the leading tilde to the name of the user's home directory: //
////////////////////////////////////////////////////////////////////// ////
static string convertDirectoryNameToExactDirectoryName(string dir) {
    if (dir.size() == 0) {
        throw "The directory name is empty. This should not happen.";
    }

    if (dir[0] != '~') {
        // Nothing to do.
        return dir;
    }

    string homeDirectory = getHomeDirectory();
    auto iter = dir.cbegin();
    std::advance(iter, 1);

    for (auto it = iter; it != dir.cend(); ++it) {
        homeDirectory.push_back(*it);
    }

    return homeDirectory;
}

static void checkIfstream(ifstream& ifs) {
    if (!ifs.is_open()) {
        throw "Could not open the tag file for reading.";
    }

    if (!ifs.good()) {
        throw "The tag file input stream is not good.";
    }
}

static void checkOfstream(ofstream& ofs) {
    if (!ofs.is_open()) {
        throw "Could not open the tag file for writing.";
    }

    if (!ofs.good()) {
        throw "The tag file output stream is not good.";
    }
}

//// //////////////////////////////////////////////////////////////////
 // Jumps to the directory to which dt was switching most recently: //
////////////////////////////////////////////////////////////////// ////
static void jumpToPreviousDirectory() {
    string tagFilePath = getTagFilePath();
    ifstream ifs;
    ifs.open(tagFilePath, std::ifstream::in);
    checkIfstream(ifs);

    DirectoryTagEntryList directoryTagEntryList;
    ifs >> directoryTagEntryList;

    DirectoryTagEntry* directoryTagEntry = directoryTagEntryList[PREV_TAG_NAME];
    string nextPath;
    string currentWorkingDirectory = getCurrentWorkingDirectory();

    if (directoryTagEntry->getTagName().compare(PREV_TAG_NAME) == 0)  {
        nextPath = updatePreviousDirectory(directoryTagEntryList, 
                                           currentWorkingDirectory);
    } else {
        DirectoryTagEntry prevTagEntry(PREV_TAG_NAME,
                                       currentWorkingDirectory);

        directoryTagEntryList << prevTagEntry;
        nextPath = currentWorkingDirectory;
    }

    ofstream ofs;
    ofs.open(tagFilePath, ofstream::out);
    checkOfstream(ofs);

    directoryTagEntryList >> ofs;
    ofs.close();

    cout << OPERATION_SWITCH_DIRECTORY
         << '\n'
         << convertDirectoryNameToExactDirectoryName(nextPath)
         << std::flush;
}

//// /////////////////////////////
 // Switches to the directory: //
///////////////////////////// ////
static void switchDirectory(std::string const& tag) {
    DirectoryTagEntryList directoryTagEntryList;
    std::ifstream ifs;
    std::string tagFilePath = getTagFilePath();
    ifs.open(tagFilePath, std::ifstream::in);
    checkIfstream(ifs);

    ifs >> directoryTagEntryList;

    string currentWorkingDirectory = getCurrentWorkingDirectory();

    if (directoryTagEntryList.size() == 0) {
        string nextDirectory = 
            updatePreviousDirectory(
                directoryTagEntryList,
                currentWorkingDirectory);

        std::ofstream ofs;
        ofs.open(getTagFilePath(), std::ofstream::out);
        checkOfstream(ofs);

        directoryTagEntryList >> ofs;
        ofs.close();

        cout << OPERATION_SWITCH_DIRECTORY
             << '\n'
             << convertDirectoryNameToExactDirectoryName(nextDirectory);
    }

    if (directoryTagEntryList.size() == 1) {
        cout << OPERATION_SWITCH_DIRECTORY
             << "\n"
             << directoryTagEntryList[0]->getDirectoryName();

        updatePreviousDirectory(directoryTagEntryList,
                                currentWorkingDirectory)  ;

        std::ofstream ofs;
        ofs.open(getTagFilePath(), std::ofstream::out);
        checkOfstream(ofs);

        directoryTagEntryList >> ofs;
        ofs.close();
    }

    DirectoryTagEntry* bestMatch = directoryTagEntryList[tag];

    std::ofstream ofs;
    ofs.open(getTagFilePath(), std::ofstream::out);
    checkOfstream(ofs);

    updatePreviousDirectory(directoryTagEntryList, 
                            currentWorkingDirectory);

    directoryTagEntryList >> ofs;
    ofs.close();

    // New line?
    cout << OPERATION_SWITCH_DIRECTORY
         << '\n'
         << convertDirectoryNameToExactDirectoryName(
                bestMatch->getDirectoryName());
}

//// //////////////////////////////////////
 // Lists taga without the directories: //
////////////////////////////////////// ////
static void listTagsOnly(
    DirectoryTagEntryList const& directoryTagEntryList) {

    cout << OPERATION_DESCRIPTOR_SHOW_TAG_ENTRY_LIST << '\n';

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

//// //////////////////////////////
 // Lists taga and directories: //
////////////////////////////// ////
static void listTagsAndDirectories(
        DirectoryTagEntryList const& directoryTagEntryList) {
    cout << OPERATION_DESCRIPTOR_SHOW_TAG_ENTRY_LIST << '\n';

    size_t maxTagLength = getMaximumTagLength(directoryTagEntryList);

    for (size_t index = 0, sz = directoryTagEntryList.size(); 
         index < sz; 
         index++) {
        DirectoryTagEntry const& directoryTagEntry =
            directoryTagEntryList.at(index);

        std::cout << std::setw(maxTagLength + 1)
                  << directoryTagEntry.getTagName() 
                  << ' '
                  << directoryTagEntry.getDirectoryName()
                  << '\n';
    }
}

//// /////////////////////////////////
 // Decided how to print the tags: //
///////////////////////////////// ////
static void listTags(std::string const& flag) {
    std::ifstream ifs;
    ifs.open(getTagFilePath(), std::ifstream::in);
    checkIfstream(ifs);

    DirectoryTagEntryList directoryTagEntryList;
    ifs >> directoryTagEntryList;

    if (flag == FLAG_LIST_TAGS_SORTED 
        || flag == FLAG_LIST_BOTH_SORTED) {
        directoryTagEntryList.sortByTags();
    } else if (flag == FLAG_LIST_BOTH_SORTED_DIRS) {
        directoryTagEntryList.sortByDirectories();
    }

    if (flag == FLAG_LIST_TAGS_SORTED
        || flag == FLAG_LIST_TAGS) {
        listTagsOnly(directoryTagEntryList);
    } else if (flag == FLAG_LIST_BOTH 
        || flag == FLAG_LIST_BOTH_SORTED
        || flag == FLAG_LIST_BOTH_SORTED_DIRS) {
        listTagsAndDirectories(directoryTagEntryList);
    }
}

//// ///////////////////////////////
 // Processes a single flag/tag: //
/////////////////////////////// ////
static void processSingleFlag(std::string const& arg) {
    if (arg == FLAG_LIST_BOTH
        || arg == FLAG_LIST_TAGS
        || arg == FLAG_LIST_TAGS_SORTED
        || arg == FLAG_LIST_BOTH_SORTED
        || arg == FLAG_LIST_BOTH_SORTED_DIRS) {
        listTags(arg);
    } else {
        switchDirectory(arg);
    }
}

static void processUpdatePrevious(string& dir) {
    string tagFilePath = getTagFilePath();
    DirectoryTagEntryList directoryTagEntryList;

    ifstream ifs;
    ifs.open(tagFilePath, ifstream::in);
    checkIfstream(ifs);

    ifs >> directoryTagEntryList;
    ifs.close();

    DirectoryTagEntry* previousEntryCandidate = 
        directoryTagEntryList[PREV_TAG_NAME];

    string nextPath;

    if (previousEntryCandidate->getTagName().compare(PREV_TAG_NAME) == 0) {
        nextPath = previousEntryCandidate->getDirectoryName();
        previousEntryCandidate->setDirectoryName(dir);
    } else {
        DirectoryTagEntry prevDirectoryTagEntry(PREV_TAG_NAME, dir);
        nextPath = getCurrentWorkingDirectory();
        directoryTagEntryList << prevDirectoryTagEntry;
    }

    ofstream ofs;
    ofs.open(getTagFilePath(), ofstream::out);
    checkOfstream(ofs);
    directoryTagEntryList >> ofs;
    ofs.close();

    // return nextPath;
}

int main(int argc, char *argv[]) {
    try {
        if (argc == 1) {
            jumpToPreviousDirectory();
        } else if (argc == 2) {
            processSingleFlag(argv[1]);
        } else if (argc == 3) {
            string flag = argv[1];

            if (flag.compare(FLAG_UPDATE_PREVIOUS) != 0) {
                string errorMsg = "Flag ";
                errorMsg = errorMsg.append(flag);
                errorMsg = errorMsg.append(" not recognized.");
                throw errorMsg.data();
            }

            string dir = argv[2];
            processUpdatePrevious(dir);
        }
    } catch (char *const msg) {
        cerr << msg << '\n';
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

Makefile

all: DirectoryTagEntry.cpp DirectoryTagEntryList.cpp main.cpp
    g++ -std=c++17 -Wall -Werror -pedantic -o ds_engine DirectoryTagEntry.cpp DirectoryTagEntryList.cpp main.cpp

install:
    chmod 500 installer.sh
    installer.sh

See also

  1. The bash script
  2. The installer script

Critique request

Especially, main.cpp looks like cr*p to me. How could I C++:fy it?

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

The Makefile

You are not utilizing the power of Make. Each make invocation will compile the source files and link the binary, no matter if that is necessary or not. The dependencies in

all: DirectoryTagEntry.cpp DirectoryTagEntryList.cpp main.cpp

do not help because no file named “all” is ever created. It is better to make the actual target file dependent on the sources (and the included headers).

There is also some duplication (the source files are listed twice) which is error-prone.

The first part of the Makefile then looks like this:

all:    ds_engine

SOURCES = DirectoryTagEntry.cpp DirectoryTagEntryList.cpp main.cpp
HEADERS = DirectoryTagEntry.h DirectoryTagEntryList.h

ds_engine:  $(SOURCES) $(HEADERS)
     g++ -std=c++17 -Wall -Werror -pedantic -o $@ $(SOURCES)

For larger projects this should be improved further (compile only the necessary source files, generate dependencies automatically), but here it might be sufficient.


Setting the mode of the installer shell script to 500 in the install rule looks strange to me: It makes the file read-only even for its owner. Which means that after calling “make install” once you cannot edit it anymore without reverting that change.

Common choices are 755 (readable and executable for all, writeable only for the owner) or perhaps 700 (full access for the owner, no access for all others). With a “symbolic mode” (e.g. rwxr-xr-x) you can make the intention even clearer.

But actually I would avoid changing the mode of a file under source control at at all. With

install:
    sh installer.sh

this is no longer needed.

\$\endgroup\$
1
\$\begingroup\$

Creating a matrix

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.

Wrong operator overloads for DirectoryTagEntryList?

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.

Useless const return value

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.

Declarations of functions without an implementation

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.

Avoid C string manipulation

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().

Use std::filesystem functions

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.

Missing error checking

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.

Make your classes suitable for range-for loops

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();
   }
   ...
};

Don't throw raw strings

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;
}

Use getopt_long() for option parsing

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.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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