6
\$\begingroup\$

I have this repository.

The idea is that there is a file tags sitting in the users home directory, and which contains the tag declarations of the format <tag> <directory>. For example, the tags file may look like:

home C:\Users\bob
docs C:\Users\bob\Documents
down C:\Users\bob\Downloads
croot C:\

Now, when the user types ds down, Bob's command line changes the current directory to C:\Users\bob\Downloads.

Also, if there is no match for the input tag, we use Levenshtein distance in order to find the closest tag and use it as the requested tag.

Finally, ds remembers the previous directory. So, when the user types simply ds in the command line, it jumps back and forth between the two most recent directories. This allows, faster transition between two directories.

The main component is the directory tag engine, which follows:

#define _CRT_SECURE_NO_WARNINGS

#include <algorithm>
#include <cctype>
#include <cstdlib>
#include <cstring>
#include <fstream>
#include <iostream>
#include <ostream>
#include <string>
#include <ShlObj.h>
#include <vector>

using std::cerr;
using std::cout;
using std::find_if;
using std::getline;
using std::ifstream;
using std::ios_base;
using std::isspace;
using std::istream;
using std::malloc;
using std::min;
using std::ofstream;
using std::ostream;
using std::size_t;
using std::strcpy;
using std::strlen;
using std::string;
using std::vector;

static const string TAG_FILE_NAME = "tags";
static const string COMMAND_FILE_NAME = "ds_command.cmd";
static const string PREVIOUS_DIRECTORY_TAG_NAME = "previous_directory";

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

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

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

static char* get_home_directory_name() {
    CHAR path[MAX_PATH];
    // Load the home path name:
    SHGetFolderPathA(NULL, CSIDL_PROFILE, NULL, 0, path);
    // Get the length of the home path name:
    size_t len = strlen(path);
    // Allocate space for the home path name copy:
    char* copy_path = new char[len + 1];
    // Copy the home path name:
    strcpy(copy_path, path);
    // Zero-terminate the home path name copy:
    copy_path[len] = '\0';

    return copy_path;
}

static ifstream get_tag_file_ifstream(string const& tag_file_path) {
    ifstream ifs(tag_file_path);
    return ifs;
}

static ofstream get_tag_file_ofstream(string const& tag_file_path) {
    ofstream ofs(tag_file_path, ios_base::trunc);
    return ofs;
}

static ofstream get_command_file_stream(string const& command_file_path) {
    ofstream ofs(command_file_path, ios_base::trunc);
    return ofs;
}

static size_t levenshtein_distance(string const& str1, string const& str2) {
    vector<vector<size_t>> matrix;

    // Initialize the matrix setting all entries to zero:
    for (size_t row_index = 0; row_index <= str1.length(); row_index++) {
        vector<size_t> row_vector;
        row_vector.resize(str2.length() + 1);
        matrix.push_back(row_vector);
    }

    // Initialize the leftmost column:
    for (size_t row_index = 1; row_index <= str1.length(); row_index++) {
        matrix.at(row_index).at(0) = row_index;
    }

    // Initialize the topmost row:
    for (size_t column_index = 1;
        column_index <= str2.length();
        column_index++) {
        matrix.at(0).at(column_index) = column_index;
    }

    for (size_t column_index = 1;
        column_index <= str2.length();
        column_index++) {

        for (size_t row_index = 1; row_index <= str1.length(); row_index++) {
            size_t substitution_cost;

            if (str1.at(row_index - 1) == str2.at(column_index - 1)) {
                substitution_cost = 0;
            }
            else {
                substitution_cost = 1;
            }

            matrix.at(row_index).at(column_index) =
                min(min(matrix.at(row_index - 1).at(column_index) + 1,
                    matrix.at(row_index).at(column_index - 1) + 1),
                    matrix.at(row_index - 1)
                    .at(column_index - 1) + substitution_cost);
        }
    }

    return matrix.at(str1.length()).at(str2.length());
}

static string get_current_directory_name() {
    char path_name[MAX_PATH];
    GetCurrentDirectoryA(MAX_PATH - 1, path_name);
    return string(path_name);
}

class DirectoryTagEntry {
private:
    string tag;
    string dir;

public:
    DirectoryTagEntry(string tag_, string dir_) :
        tag{ tag_ },
        dir{ dir_ }
    {

    }

    string get_tag() const {
        return tag;
    }

    string get_dir() const {
        return dir;
    }

    void set_dir(string const& new_dir) {
        dir = new_dir;
    }
};

std::ostream& operator << (ostream& os, DirectoryTagEntry const& dte) {
    os << dte.get_tag() << " " << dte.get_dir();
    return os;
}

std::istream& operator >> (istream& is, vector<DirectoryTagEntry>& entries) {
    string tag;
    string dir;
    is >> tag;
    getline(is, dir);
    trim(tag);
    trim(dir);
    DirectoryTagEntry dte(tag, dir);
    entries.push_back(dte);
    return is;
}

static string get_tag_file_path() {
    return get_home_directory_name() + string("\\") + TAG_FILE_NAME;
}

static string get_cmd_file_path() {
    return get_home_directory_name() + string("\\") + COMMAND_FILE_NAME;
}

static bool tag_matches_previous_directory_tag(DirectoryTagEntry& entry) {
    return entry.get_tag() == PREVIOUS_DIRECTORY_TAG_NAME;
}

static vector<DirectoryTagEntry>::iterator
find_previous_directory_tag_entry_iterator(
    vector<DirectoryTagEntry>& entries) {
    return find_if(entries.begin(),
        entries.end(),
        [](DirectoryTagEntry& entry) {
            return tag_matches_previous_directory_tag(entry);
        });
}

static void process_previous_no_tag_entry(vector<DirectoryTagEntry>& entries) {
    string current_path = get_current_directory_name();
    DirectoryTagEntry dte(PREVIOUS_DIRECTORY_TAG_NAME, current_path);
    entries.push_back(dte);

    ofstream tag_file_ofs = get_tag_file_ofstream(get_tag_file_path());
    ofstream cmd_file_ofs = get_command_file_stream(get_cmd_file_path());
    size_t index = 0;

    for (DirectoryTagEntry const& dte : entries) {
        tag_file_ofs << dte.get_tag() << " " << dte.get_dir();

        if (index < entries.size() - 1) {
            tag_file_ofs << "\n";
        }

        index++;
    }

    DirectoryTagEntry current_directory_tag_entry(PREVIOUS_DIRECTORY_TAG_NAME,
        get_current_directory_name());

    tag_file_ofs << current_directory_tag_entry;

    cmd_file_ofs << "echo \"Set previous directory to ^\""
        << current_path
        << "^\".";

    tag_file_ofs.close();
    cmd_file_ofs.close();
}

static void save_tag_file(vector<DirectoryTagEntry> const& entries,
    ofstream& ofs) {
    size_t index = 0;

    for (DirectoryTagEntry const& entry : entries) {
        ofs << entry.get_tag() << " " << entry.get_dir();

        if (index < entries.size() - 1) {
            ofs << "\n";
        }

        index++;
    }
}

static void process_switch_to_previous(
    vector<DirectoryTagEntry>& entries,
    vector<DirectoryTagEntry>::iterator
    previous_directory_tag_entry_const_iterator) {

    string next_directory_path =
        (*previous_directory_tag_entry_const_iterator).get_dir();

    ofstream cmd_file_ofs = get_command_file_stream(get_cmd_file_path());

    cmd_file_ofs << "cd " << next_directory_path;
    cmd_file_ofs.close();

    string current_directory_path = get_current_directory_name();

    (*previous_directory_tag_entry_const_iterator)
        .set_dir(current_directory_path);

    ofstream tag_file_ofs = get_tag_file_ofstream(get_tag_file_path());

    save_tag_file(entries, tag_file_ofs);
    tag_file_ofs.close();
}

static void process_previous() {
    vector<DirectoryTagEntry> entries;
    string tag_file_path = get_tag_file_path();
    ifstream ifs = get_tag_file_ifstream(tag_file_path);

    while (!ifs.eof() && ifs.good()) {
        ifs >> entries;
    }

    ifs.close();

    vector<DirectoryTagEntry>::iterator
        previous_directory_tag_entry_iterator =
        find_previous_directory_tag_entry_iterator(entries);

    if (previous_directory_tag_entry_iterator == entries.cend()) {
        process_previous_no_tag_entry(entries);
    }
    else {
        process_switch_to_previous(entries, previous_directory_tag_entry_iterator);
    }
}

static void create_previous_tag_entry() {
    string current_directory_path = get_current_directory_name();
    ifstream ifs = get_tag_file_ifstream(get_tag_file_path());
    vector<DirectoryTagEntry> entries;
    bool previous_entry_updated = false;

    while (!ifs.eof() && ifs.good()) {
        string tag;
        string dir;

        ifs >> tag;
        getline(ifs, dir);

        trim(tag);
        trim(dir);

        if (tag == PREVIOUS_DIRECTORY_TAG_NAME) {
            dir = current_directory_path;
            previous_entry_updated = true;
        }

        DirectoryTagEntry dte(tag, dir);
        entries.push_back(dte);
    }

    if (!previous_entry_updated) {
        DirectoryTagEntry dte(PREVIOUS_DIRECTORY_TAG_NAME,
            current_directory_path);

        entries.push_back(dte);
    }

    ifs.close();

    ofstream ofs = get_tag_file_ofstream(get_tag_file_path());
    size_t index = 0;

    for (DirectoryTagEntry const& entry : entries) {
        ofs << entry.get_tag() << " " << entry.get_dir();

        if (index < entries.size() - 1) {
            ofs << "\n";
        }

        index++;
    }

    ofs.close();
}

int main(int argc, char* argv[]) {
    if (argc == 1) {
        process_previous();
        return EXIT_SUCCESS;
    }

    if (argc != 2) {
        ofstream ofs = get_command_file_stream(get_cmd_file_path());
        ofs << "@echo off\n";
        ofs << "echo|set /p=\"Expected 1 argument. "
            << (argc - 1) << " received.\"\n";

        return 1;
    }

    create_previous_tag_entry();

    string target_tag = argv[1];
    string tag_file_name = get_tag_file_path();
    string cmd_file_name = get_cmd_file_path();

    ifstream ifs(tag_file_name);
    ofstream ofs = get_command_file_stream(get_cmd_file_path());
    size_t best_known_levenshtein_distance = SIZE_MAX;
    string best_known_directory;

    if (ifs.eof()) {
        ofs << "echo|set /p=\"The tag file is empty. Please edit ^\""
            << get_home_directory_name() << "\\tags^\"";

        ofs.close();
        ifs.close();
        return 0;
    }

    while (!ifs.eof() && ifs.good()) {
        string tag;
        string dir;

        ifs >> tag;
        getline(ifs, dir);

        trim(tag);
        trim(dir);

        size_t levenshtein_dist = levenshtein_distance(tag, target_tag);

        if (levenshtein_dist == 0) {
            best_known_directory = dir;
            break;
        }

        if (best_known_levenshtein_distance > levenshtein_dist) {
            best_known_levenshtein_distance = levenshtein_dist;
            best_known_directory = dir;
        }
    }

    ofs << "cd " << best_known_directory;
    ofs.close();
    ifs.close();
    return 0;
}

Critique request

Since it looks like 5h17 to me, I would like to read comments on the following topics:

  1. Overall program design,
  2. Naming,
  3. Efficiency,
  4. (How about move semantics),
  5. Const-correctness.
  6. Commenting.

Thank you for your time.

\$\endgroup\$
3

2 Answers 2

6
\$\begingroup\$

Reconsider Your Imports

Currently, you’re importing pretty much every identifier from std:: that you use. I actively avoid this for names like string and vector that have been used many times for different things. To paraphrase one great answer I read about this in the past, “Whenever I see std::string, I know that this code is using the STL. When I just see string, I’m not sure.”

My own personal preference is to import identifiers that didn’t originally have std:: in front of them (that is, either older than C++98 or from C99) and that I learned to use without std:: in the ’90s. You never have to worry that someone else’s code uses the names cout or size_t for anything else. The same is not true for find_if. Many other people, though, just use std:: consistently. If you’re not some old fogey who still writes cout because that’s how I’ve done it for thirty years, always using std:: will be easier to remember.

If you do keep the long list of imports, you can turn them into a list instead of one per line:

using std::cerr, std::cout, std::min, std::isspace, std::istream, std::ofstream,
      std::ostream, std::size_t;

Use std::filesystem

There’s been filesystem support in the standard library since C++17 that’s more portable than ShlObj.h, and more modern in many ways. For example, it supports standard strings. Additionally, you’re using 8-bit versions of Windows API calls that don’t support Unicode filenames.

Use RAII. Nobody Normally Needs Naked new

You do a lot of manual memory management. Some of it is forced on you by the Windows API. However, right now, you’re returning a char[] that you allocate with new instead of a std::string, which is just a bug waiting to happen. Worse, you mix malloc, new and array new, which all must be deleted manually with different syntax, with no documentation of which one the user is supposed to be using!

Similarly, you should be using input functions that read into a std::string, or other object that manages is memory with RAII, not a fixed-size buffer.

You should change the Windows API calls to standard library calls if those meet your requirements, and stop returning raw pointers. Never write a delete or free outside a destructor, if you can possibly avoid it. Return either a smart pointer or a container object. Even if you need to free its memory manually before the object can be destroyed, for some reason, you can clear it.

\$\endgroup\$
3
  • \$\begingroup\$ Nice one. However, could you supply some short snippets on how to: (1) get the current directory via std::filesystem, and (2) the home directory via the same C++17 facility supporting, say, UTF-8? \$\endgroup\$
    – coderodde
    Commented May 24, 2023 at 5:39
  • \$\begingroup\$ @coderodde Respectively, std::filesystem::current_path. and std::filesystem::absolute on the pathname you retrieve in an OS-specific way, often getenv (the %UsERPROFILE% environment variable on Windows, or $HOME on Linux). \$\endgroup\$
    – Davislor
    Commented May 24, 2023 at 6:06
  • \$\begingroup\$ @coderodde There are a few different ways to get a pathname in UTF-8. The std::filesystem::path::u8string member function returns it in UTF-8-encoded native format as a std::u8string. \$\endgroup\$
    – Davislor
    Commented May 24, 2023 at 6:24
1
\$\begingroup\$

Environment variables

Home directory and home drives are passed to the application as environment variables and reading those seemed more readable to me. I would also incorporate @Davislor's suggestion about using std::filesystem:

fs::path get_home_directory_name() {
    const auto homepath = std::getenv("homepath");
    const auto homedrive = std::getenv("homedrive");

    if (!homepath || !homedrive) {
        throw std::runtime_error("Either homepath or homedrive env variables are undefined");
    }

    const auto native_format = fs::path::format::native_format;
    return fs::path(homedrive, native_format) / fs::path(homepath, native_format);
}

Tag

I believe some changes to it would make some operations a lot more readable. I propose the following:

        struct Tag {
            std::string name;
            fs::path value;

            Tag(const std::string& name, const fs::path& value):
                name(name),
                value(value) {}

            static Tag from_line(std::string_view line);
            std::size_t distance_to(std::string_view target_tag) const;

            std::string to_string() const {
                return std::format("{} {}", name, value.string());
            }

        private:
            Tag() = default;
        };

This would make finding closest tag look like this:

        // assume tags cannot be empty
        auto min_element = std::ranges::min_element(tags, [target_tag](const Tag& lhs, const Tag& rhs) {
            return lhs.distance_to(target_tag) < rhs.distance_to(target_tag);
        });

        return min_element->value;

Although the original code does not read more than needed and stops early, I believe the code above is more readable due to usage of standard library functions and being shorter.

The parsing of the tag entry file could look like this:

    Tags::Tag Tags::Tag::from_line(std::string_view line) {
        auto iss = std::istringstream(std::string(line));
        Tag tag{};
        std::string path_str;
        iss >> tag.name >> path_str; // assume file has valid contents, otherwise use a library for parsing
        tag.value = fs::path(path_str, fs::path::format::native_format);

        return tag;
    }

    // then, somewhere in a function
        auto tags_file = std::ifstream(home / fs::path("tags"));
        std::string line;
        while (std::getline(tags_file, line)) {
            tags.push_back(Tag::from_line(line));
        }

Tests

It would be great to have tests, but I understand that time was limited. Having tests greatly increases confidence in making changes in the code, making them easy to execute doubles that.

Features

This might be a bit unexpected, but I believe designing an application that offers more streamlined (as in fewer, but powerful) features would make the code quite a bit simpler.

  • home and root could be built-in and reserved tag names (this eliminates the need to check ).
  • the previous tag entry could be written on its own into a temporary directory, see temp directory.

Even more straightforward solution is to write a powershell script that creates the variables from a tag entry file and let people keep using powershell.

\$\endgroup\$

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.