1
\$\begingroup\$

This is a rework of a small program for faster(?) switching working directories, and it looks like a pile of crap, I must admit. Could you give me pointers how to make it maintainable?

dt_tag_entry.hpp

#ifndef NET_CODERODDE_DT2_TAG_ENTRY_HPP
#define NET_CODERODDE_DT2_TAG_ENTRY_HPP

#include <string>

namespace net {
namespace coderodde {
namespace dt {

    class TagEntry {
    public:
        TagEntry(std::string const& tag, std::string const& directory);
        TagEntry();
        TagEntry(TagEntry const& other);
        TagEntry& operator=(TagEntry&& other);
        TagEntry& operator=(TagEntry const& other);
        std::string const& getTag() const;
        std::string const& getDirectory() const;
        void setTag(std::string& tag);
        void setDirectory(std::string& directory);

    private:
        std::string m_tag;
        std::string m_directory;
    };

} // End of namespace 'net::coderodde::dt2'.
} // End of namespace 'net::coderodde'.
} // End of namespace 'net'.

#endif // NET_CODERODDE_DT2_TAG_ENTRY_HPP

dt_tag_entry.cpp

#include "dt_tag_entry.hpp"
#include <string>
#include <utility>

namespace net {
namespace coderodde {
namespace dt {

    TagEntry::TagEntry(std::string const& tag,
                       std::string const& directory)
        :
        m_tag(tag),
        m_directory(directory) {

    }

    TagEntry::TagEntry()
        :
        m_tag(""),
        m_directory("") {

    }

    TagEntry::TagEntry(TagEntry const& other)
        :
        m_tag(other.m_tag),
        m_directory(other.m_directory) {

    }

    TagEntry& TagEntry::operator=(TagEntry&& other) {
        m_tag = std::move(other.m_tag);
        m_directory = std::move(other.m_directory);
        return *this;
    }

    TagEntry& TagEntry::operator=(TagEntry const& other) {
        m_tag = other.m_tag;
        m_directory = other.m_directory;
        return *this;
    }

    std::string const& TagEntry::getTag() const {
        return m_tag;
    }

    std::string const& TagEntry::getDirectory() const {
        return m_directory;
    }

    void TagEntry::setTag(std::string& tag) {
        m_tag = tag;
    }

    void TagEntry::setDirectory(std::string& directory) {
        m_directory = directory;
    }

} // End of namespace 'net::coderodde::dt2'.
} // End of namespace 'net::coderodde'.
} // End of namespace 'net'.

dt_tag_entry_list.hpp

#ifndef NET_CODERODDE_DT2_TAG_ENTRY_LIST_HPP
#define NET_CODERODDE_DT2_TAG_ENTRY_LIST_HPP

#include "dt_tag_entry.hpp"
#include <vector>

using net::coderodde::dt::TagEntry;

namespace net {
namespace coderodde {
namespace dt {

    class TagEntryList {
    public:
        void operator<<(TagEntry const& tagEntry);
        TagEntry operator[](std::string const& tag) const;
        std::vector<TagEntry>::const_iterator begin() const;
        std::vector<TagEntry>::const_iterator end() const;
        void sortByTags();
        void sortByDirectories();
        bool empty() const;
        std::size_t size() const;

    private:
        std::vector<TagEntry> m_entries;
    };

} // End of namespace 'net::coderodde::dt2'.
} // End of namespace 'net::coderodde'.
} // End of namespace 'net'.

#endif // NET_CODERODDE_DT2_TAG_ENTRY_LIST_HPP

dt_tag_entry_list.cpp

#include "dt_tag_entry.hpp"
#include "dt_tag_entry_list.hpp"
#include <algorithm>
#include <limits>
#include <stdexcept>

using net::coderodde::dt::TagEntry;

static size_t computeEditDistance(std::string const& string1,
                                  std::string const& string2,
                                  size_t index1,
                                  size_t index2) {
    if (index1 == 0) {
        return index2;
    } else if (index2 == 0) {
        return index1;
    }

    return std::min(
            std::min(
                computeEditDistance(string1, string2, index1 - 1, index2) + 1,
                computeEditDistance(string1, string2, index1, index2 - 1) + 1
            ),
                computeEditDistance(string1, 
                                    string2, 
                                    index1 - 1, 
                                    index2 - 1) +
                (string1[index1 - 1] != string2[index2 - 1] ? 1 : 0)
    );
}

static size_t computeEditDistance(std::string const& string1, std::string const& string2) {
    return computeEditDistance(string1,
                               string2,
                               string1.length(),
                               string2.length());
}

namespace net {
namespace coderodde {
namespace dt {

    void TagEntryList::operator<<(TagEntry const& tagEntry) {
        m_entries.push_back(tagEntry);
    }

    TagEntry TagEntryList::operator[](std::string const& tag) const {
        if (m_entries.empty()) {
            throw std::runtime_error("No entries available.");
        }

        TagEntry bestTagEntry;
        size_t bestEditDistance = std::numeric_limits<size_t>::max();

        for (TagEntry const& tagEntry : m_entries) {
            size_t currentEditDistance = computeEditDistance(tag, tagEntry.getTag());

            if (bestEditDistance > currentEditDistance) {
                bestEditDistance = currentEditDistance;
                bestTagEntry = tagEntry;
            }
        }

        return bestTagEntry;
    }

    std::size_t TagEntryList::size() const {
        return m_entries.size();
    }

    std::vector<TagEntry>::const_iterator TagEntryList::begin() const {
        return m_entries.cbegin();
    }

    std::vector<TagEntry>::const_iterator TagEntryList::end() const {
        return m_entries.cend();
    }

    void TagEntryList::sortByTags() {
        std::stable_sort(m_entries.begin(),
            m_entries.end(),
            [](TagEntry const& tagEntry1,
               TagEntry const& tagEntry2) {
            return tagEntry1.getTag() < tagEntry2.getTag();
        });
    }

    void TagEntryList::sortByDirectories() {
        std::stable_sort(m_entries.begin(),
            m_entries.end(),
            [](TagEntry const& tagEntry1,
               TagEntry const& tagEntry2) {
            return tagEntry1.getDirectory() < tagEntry2.getDirectory();
        });
    }

    bool TagEntryList::empty() const {
        return m_entries.empty();
    }

} // End of namespace 'net::coderodde::dt2'.
} // End of namespace 'net::coderodde'.
} // End of namespace 'net'.

main.cpp

#include <linux/limits.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <pwd.h>
#include "dt_tag_entry.hpp"
#include "dt_tag_entry_list.hpp"
#include <algorithm>
#include <cctype>
#include <cstdlib>
#include <fstream>
#include <iomanip>
#include <iostream>
#include <locale>
#include <sstream>
#include <stdexcept>
#include <string>
#include <utility>

const std::string FLAG_LIST_TAGS_AND_DIRECTORIES                = "-L";
const std::string FLAG_LIST_TAGS_NO_DIRECTORIES                 = "-l";
const std::string FLAG_LIST_TAGS_AND_DIRECTORIES_SORTED         = "-S";
const std::string FLAG_LIST_TAGS_NO_DIRECTORIES_SORTED          = "-s";
const std::string FLAG_LIST_TAGS_AND_DIRECTORIES_SORTED_BY_DIRS = "-d";
const std::string TAG_ENTRY_LIST_FILE_DIRECTORY                 = "/.dt";
const std::string TAG_ENTRY_LIST_FILE_NAME                      = "/tags";
const std::string TAG_LINE                                      = "tag";
const std::string OPERATION_DESCRIPTOR_SWITCH_DIRECTORY         = "switch_directory";
const std::string OPERATION_DESCRIPTOR_SHOW_TAG_ENTRY_LIST      = "show_tag_entry_list";
const std::string OPERATION_DESCRIPTOR_MESSAGE                  = "message";
const std::string LIST_LINE                                     = "list";
const size_t LINE_BUFFER_CAPACITY = 1024;

////// https://stackoverflow.com/questions/216823/whats-the-best-way-to-trim-stdstring
static inline void ltrim(std::string &s) {
    s.erase(s.begin(), std::find_if(s.begin(), s.end(), [](int ch) {
        return !std::isspace(ch);
    }));
}

static inline void rtrim(std::string &s) {
    s.erase(std::find_if(s.rbegin(), s.rend(), [](int ch) {
        return !std::isspace(ch);
    }).base(), s.end());
}

inline void trim(std::string &s) {
    ltrim(s);
    rtrim(s);
}

using net::coderodde::dt::TagEntry;
using net::coderodde::dt::TagEntryList;

static void operator>>(std::ifstream& inputFileStream, 
                       TagEntryList& tagEntryList) {
    char lineBuffer[LINE_BUFFER_CAPACITY];

    while (!inputFileStream.eof() && !inputFileStream.bad() && !inputFileStream.fail()) {
        inputFileStream.getline(lineBuffer, LINE_BUFFER_CAPACITY);
        std::stringstream ss;
        std::string tag;
        std::string directory;
        ss << lineBuffer;
        ss >> tag;
        char directoryBuffer[PATH_MAX];
        ss.getline(directoryBuffer, PATH_MAX);
        directory = directoryBuffer;
        trim(directory);

        if (!tag.empty() && !directory.empty()) {
                TagEntry tagEntry(tag, directory);
                tagEntryList << tagEntry;
        }
    }
}

static size_t getMaximumTagLength(TagEntryList const& tagEntryList) {
    auto const& maximumLengthTagIter =
        std::max_element(tagEntryList.begin(),
                tagEntryList.end(),
                [](TagEntry const& tagEntry1,
                        TagEntry const& tagEntry2) {
        return tagEntry1.getTag().length() <
                tagEntry2.getTag().length();
    });

    return maximumLengthTagIter->getTag().length();
}

static void listTags(TagEntryList const& tagEntryList) {
    std::cout << OPERATION_DESCRIPTOR_SHOW_TAG_ENTRY_LIST
              << '\n';

    for (TagEntry const& tagEntry : tagEntryList) {
        std::cout << tagEntry.getTag() << "\n";
    }
}

static void listTagsAndDirectories(TagEntryList const& tagEntryList) {
    if (tagEntryList.empty()) {
        // getMaximumTagLength assumes that the tagEntryList
        // is not empty. (It would dereference end().
        std::cout << OPERATION_DESCRIPTOR_MESSAGE 
                  << "\nTag list is empty.\n";
        return;
    }

    size_t maximumTagLength = getMaximumTagLength(tagEntryList);
    std::cout << OPERATION_DESCRIPTOR_SHOW_TAG_ENTRY_LIST
              << '\n';

    for (TagEntry const& tagEntry : tagEntryList) {
        std::cout << std::setw(maximumTagLength) 
                  << std::left 
                  << tagEntry.getTag()
                  << " "
                  << tagEntry.getDirectory() 
                  << "\n";
    }
}

static std::string matchTag(TagEntryList const& tagEntryList, std::string const& tag) {
    try {
        TagEntry bestTagEntry = tagEntryList[tag];
        return bestTagEntry.getDirectory();
    }
    catch (std::runtime_error const& err) {
        std::exit(1);
    }
}

static std::string getUserHomeDirectoryName() {
    struct passwd* pw = getpwuid(getuid());
    std::string name{pw->pw_dir}; 
    return name;
}

static void createTagFile(char* fileName) {
    int fd = open(fileName, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
    close(fd);
}

static TagEntryList loadTagFile(std::string const& tagFileName) {
    std::ifstream ifs;
    ifs.open(tagFileName.c_str(), std::ifstream::in);
    TagEntryList tagEntryList;
    ifs >> tagEntryList;
    ifs.close();
    return tagEntryList;
}

static void toggleDirectory(std::string const& tagFileName) {
    TagEntryList tagEntryList = loadTagFile(tagFileName);
    std::string path;

    try {
        path = tagEntryList["prev"].getDirectory();
    } catch (...) {
        std::cout << OPERATION_DESCRIPTOR_MESSAGE 
                  << "\nNo 'prev' tag in the tag file.\n";
        return;
    }

    std::cout << OPERATION_DESCRIPTOR_SWITCH_DIRECTORY
              << '\n'
              << path;
}

static std::string getTagFilePath() {
    std::string homeDirectoryName = getUserHomeDirectoryName();
    std::string directoryTaggerDirectory = 
            homeDirectoryName + TAG_ENTRY_LIST_FILE_DIRECTORY;
    std::string tagFilePath =
            directoryTaggerDirectory + TAG_ENTRY_LIST_FILE_NAME;
    return tagFilePath;
}

static std::string omitTilde(std::string& dir,
                             std::string const& homeDirectoryPath) {
    if (dir[0] == '~') {
        return homeDirectoryPath + dir.substr(1);
    } else {
        return dir;
    }
}

int main(int argc, char* argv[]) {
    if (argc > 3) {
        std::cout << OPERATION_DESCRIPTOR_MESSAGE
                  << "\nInvalid number of arguments: "
                  << argc - 1
                  << '\n';

        return EXIT_FAILURE;
    }

    std::string tagFilePath = getTagFilePath(); 
    createTagFile((char*) tagFilePath.c_str()); // Make sure the tag file exists.

    // TODO: I don't think I need this.
    if (argc == 1) {
        toggleDirectory(tagFilePath);
        return EXIT_SUCCESS;
    }

    TagEntryList tagEntryList = loadTagFile(tagFilePath);

    if (argc == 3) {
        std::string flag = argv[1];
        std::string path = argv[2];

        if (std::string{"--update-prev"}.compare(flag) != 0) {
            std::cout << OPERATION_DESCRIPTOR_MESSAGE 
                      << " \n--update_prev expected, but "
                      << flag
                      << " was received.\n";

            return EXIT_FAILURE;
        }

        // Now rewrite the file.
        std::ofstream ofs;
        ofs.open(tagFilePath.c_str(),
                 std::ofstream::trunc |
                 std::ofstream::out);

        bool prevTagIsPresent = false;

        for (auto iter = tagEntryList.begin();
            iter != tagEntryList.end();
            iter++) {
            TagEntry currentTagEntry = *iter;
            std::string tag = currentTagEntry.getTag();
            std::string dir = currentTagEntry.getDirectory();
            trim(dir);
            // Reset the string to the entry:
            currentTagEntry.setTag(tag);
            currentTagEntry.setDirectory(dir);

            if (currentTagEntry.getTag().compare(std::string{"prev"}) == 0) {
                currentTagEntry.setDirectory(path);
                prevTagIsPresent = true;
            }

            ofs << currentTagEntry.getTag() 
                << " "
                << currentTagEntry.getDirectory()
                << '\n';
        }

        if (!prevTagIsPresent) {
            TagEntry tagEntry("prev", path);
            tagEntryList << tagEntry;
            ofs << tagEntry.getTag()
                << ' '
                << tagEntry.getDirectory();

            std::cout << OPERATION_DESCRIPTOR_SWITCH_DIRECTORY
                      << path;
        }

        ofs.close();
        return 0;
    } 

    std::string flag{argv[1]};

    if (flag == FLAG_LIST_TAGS_AND_DIRECTORIES_SORTED
            || flag == FLAG_LIST_TAGS_NO_DIRECTORIES_SORTED) {
        tagEntryList.sortByTags();
    }
    else if (flag == FLAG_LIST_TAGS_AND_DIRECTORIES_SORTED_BY_DIRS) {
        tagEntryList.sortByDirectories();
    }

    if (flag == FLAG_LIST_TAGS_AND_DIRECTORIES
            || flag == FLAG_LIST_TAGS_AND_DIRECTORIES_SORTED
            || flag == FLAG_LIST_TAGS_AND_DIRECTORIES_SORTED_BY_DIRS) {
        listTagsAndDirectories(tagEntryList);
    }
    else if (flag == FLAG_LIST_TAGS_NO_DIRECTORIES
            || flag == FLAG_LIST_TAGS_NO_DIRECTORIES_SORTED) {
        listTags(tagEntryList);
    }
    else {
        std::string targetDirectory = tagEntryList[argv[1]].getDirectory();
        targetDirectory = 
                targetDirectory[0] == '~' ?
                    getUserHomeDirectoryName() + targetDirectory.substr(1) :
                    targetDirectory;

        std::cout << OPERATION_DESCRIPTOR_SWITCH_DIRECTORY
                  << '\n'
                  << targetDirectory;
    }

    return 0;
}

(The entire story is here.)

\$\endgroup\$
  • \$\begingroup\$ I think you could use Boost's trim methods instead of rolling your own, but that's minor. Even less importantly, I'd rename begin and end to cbegin and cend because they return const_iterators. \$\endgroup\$ – Juho Feb 19 at 21:14
2
\$\begingroup\$

It seems to me that a whole lot of this is duplicating things that are already in the standard library (and many other libraries).

It also seems to me like it does a lot of work to handle things that are more easily dealt with via a simple text editor. Just for example, if you want to add command aliases for bash, you don't feed some special command to bash to have it modify your ~/.bashrc. Instead, you use a text editor of your choice to edit ~/.bashrc, and then (most likely) re-source it to get bash to use it. I'd tend to do pretty much the same here--have the utility stick to expanding tags to directory names, and let the user edit tags with their normal text editor.

Handling things that way, the code can be reduced to something on this general order:

#include <map>
#include <iomanip>
#include <fstream>
#include <unistd.h>
#include <stdlib.h>
#include <iostream>
#include <pwd.h>

static std::string get_home_dir() {
    struct passwd* pw = getpwuid(getuid());
    std::string name{pw->pw_dir}; 
    return name;
}

// expand tilde, if needed. To be more thorough, consider using
// wordexp(3) instead.
std::string expand(std::string in) { 
    std::string home = get_home_dir();

    auto pos = in.find("~");
    if (pos != std::string::npos) {
        in.replace(pos, 1, home);
    }
    return in;
}

// Read tags from file. A tag must not contain white-space, and a "#" at the
// beginning of a line signifies a comment.
std::map<std::string, std::string> read_tags(std::string const &filename) { 
    std::ifstream in{expand(filename)};

    std::string tag;
    std::string path;
    std::map<std::string, std::string> ret;

    while (in >> tag) {
        // I'm not sure if you supported comments in the tags file, but it 
        // seems like a useful thing, and it's easy, so why not?
        if (tag[0] == '#')
            continue;
        std::getline(in, path);
        ret[tag] = path;
    }
    return ret;
}

void write_tags(std::map<std::string, std::string> const &tags, std::string const &filename) {     
    std::ofstream out{expand(filename)};
    for (auto const &p : tags)
        out << p.first << "\t" << p.second << "\n";
}

void chdir(std::string const &s) {
    // Here we write the shell script fragment to change directory
    // For now I'm going to cheat and just print the command on standard output
    std::cout << "cd " << expand(s) << "\n";
}

int main(int argc, char **argv) { 
    std::string config_path = "~/.path_tags";    

    auto tags = read_tags(config_path);

    std::string prev;
    if (tags.find("prev") != tags.end()) 
        prev = tags["prev"];

    tags["prev"] = get_current_dir_name();

    write_tags(tags, config_path);

    if (argc == 1) {
        if (!prev.empty())
            chdir(prev);
        else
            std::cerr << "No previous directory to switch to.";
    }
    else {
        auto pos = tags.find(argv[1]);
        if (pos != tags.end())
            chdir(pos->second);
    }
}

File Reading

This code:

while (!inputFileStream.eof() && !inputFileStream.bad() && !inputFileStream.fail()) {
    inputFileStream.getline(lineBuffer, LINE_BUFFER_CAPACITY);

...is pretty much broken. In fact, nearly any loop of the form while (!foo.eof()) is broken. You want to test the result of attempting to read the line instead:

while (inputFileStream.getline(lineBuffer, LINE_BUFFER_CAPACITY)) {
    // process the data we just read        
}

It's also generally easier to read the data into an std::string instead:

std::string lineBuffer;
while (std::getline(lineBuffer)) {
    // process the line
}

File I/O

Right now, you use not only iostreams, but (for no particularly obvious reason) in places use posix creat/open level functions as well. At least offhand, I don't see much reason for using both.

Tag File

Right now, you store the tags in a single global file that's shared across all users of the system. This doesn't seem like a great idea. I'd (strongly) prefer a per-user tag file (as in the code I've posted above).

default ctor

Right now, you've explicitly defined a default constructor for your TagEntry type:

TagEntry::TagEntry()
    :
    m_tag(""),
    m_directory("") {

}

When (as in this case) the compiler can generate an adequate ctor, it's generally preferable to let it do so, with a declaration like this:

TagEntry() = default;

In fact, at least at first glance, it looks like all of TagEntry's ctors are doing pretty much what the compiler-generated ones would do if you didn't define any, so it's probably better to just not declare or define any at all (but as in the code above, I'd tend to just use a map or unordered_map, and skip creating a TagEntry class at all). Right now, TagEntry is pretty much a Quasi-class, that would work about as well as a struct:

struct TagEntry { 
    std::string tag;
    std::string directory;
};

I'd say if you decide to keep it, at least keep it simple.

Reference

Pseudo-classes and Quasi-Classes

\$\endgroup\$

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.