In-memory filesystem

Question

I implemented an in-memory file system that should support basic Linux commands (mkdir, cd, mkfile...) on the current working directory. It can read commands from a file and process them.

I'm interested in a review from senior developers to see what makes a difference between a junior and senior (what should I focus on...)

#include <iostream>
#include <chrono>
#include <ctime>

#include <vector>
#include <algorithm>
#include <memory>
#include <fstream>
#include <sstream>

class Directory;

enum class EntryType
{
    FILE,
    DIRECTORY,
    ANY_TYPE
};

class Entry {
private:
    const std::string m_name;
    const std::time_t m_created;
    const EntryType   m_entryType;

    Directory *m_parentDirectory;

protected:
    Entry(const std::string &name, EntryType entryType, Directory *parentDirectory) : m_name {name},
                                                                 m_created {std::chrono::system_clock::to_time_t(std::chrono::system_clock::now())},
                                                                 m_entryType {entryType},
                                                                 m_parentDirectory {parentDirectory}
    {
    }

    virtual ~Entry()
    {
    }

public:
    const std::string& getName() const {return m_name;}
    const std::time_t& getTimeCreated() const {return m_created;}
    const EntryType& getEntryType() const {return m_entryType;}

    Directory* getParentDirectory() {return m_parentDirectory;}
};

class File : public Entry {
public:
    File(const std::string &name, Directory *parentDirectory) : Entry(name, EntryType::FILE, parentDirectory)
    {
    }
};

class Directory : public Entry {
private:
    std::vector<std::shared_ptr<Entry>> entries;

public:
    Directory(const std::string &name, Directory *parentDirectory) : Entry(name, EntryType::DIRECTORY, parentDirectory)
    {
    }

    ~Directory()
    {
    }

private:
    void list(EntryType type)
    {
        for (auto &entry : entries) {
            if (entry->getEntryType() == type)
                std::cout << getCreatedTime(entry.get()) << "\t\t" << entry->getName() << "\n";
        }
    }

    int countEntries(EntryType type)
    {
        auto count {static_cast<int>(std::count_if(entries.begin(), entries.end(), [&type](std::shared_ptr<Entry> entry){
                                            return entry->getEntryType() == type;
                                        }))};

        return count;
    }

    bool checkEntry(const std::string &name)
    {
        auto findEntry {std::find_if(entries.begin(), entries.end(), [&name](std::shared_ptr<Entry> entry){
                return entry->getName() == name;
            })};

        if (findEntry != entries.end())
            return true;

        return false;
    }

    const std::string getCreatedTime (Entry *entry) const
    {
        std::string timeCreated {std::ctime(&entry->getTimeCreated())};
        timeCreated[timeCreated.length() - 1] = '\0';

        return timeCreated;
    }

public:
    void listEntries()
    {
        std::cout << getCreatedTime(this) << "\t\t" << ".\n";

        int numDir {1};

        if (this->getParentDirectory() != nullptr) {
            std::cout << getCreatedTime(this->getParentDirectory()) << "\t\t" << "..\n";
            ++numDir;
        }

        list(EntryType::DIRECTORY);
        list(EntryType::FILE);

        std::cout << "\n";

        std::cout << countEntries(EntryType::DIRECTORY) + numDir << "\t\t" << "Dir(s)\n";
        std::cout << countEntries(EntryType::FILE) << "\t\t" << "File(s)\n";

        std::cout << "\n";
        std::cout << "--------------------------------------------------\n";
    }

    void makeDirectory(const std::string &name)
    {
        if (checkEntry(name))
            return;

        entries.push_back(std::make_shared<Directory>(name, this));
    }

    void makeFile(const std::string &name)
    {
        if (checkEntry(name))
            return;

        entries.push_back(std::make_shared<File>(name, this));
    }

    Directory* changeDirectory(const std::string &name)
    {
        if (name == ".") {
            return this;
        } else if (name == "..") {
            if (this->getParentDirectory() == nullptr) {
                return this;
            } else
                return this->getParentDirectory();
        }

        for (auto &entry : entries) {
            if (entry->getEntryType() == EntryType::DIRECTORY && entry->getName() == name)
                return static_cast<Directory *>(entry.get());
        }

        std::cout << "Directory: " << name << " does not exist\n";
        return this;
    }

    void deleteEntry(const std::string &name, EntryType type)
    {
        auto findEntry {std::find_if(entries.begin(), entries.end(), [&name](std::shared_ptr<Entry> entry){
                return entry->getName() == name;
            })};

        if (findEntry != entries.end()) {
            if (type == EntryType::ANY_TYPE || type == findEntry->get()->getEntryType()) {
                entries.erase(findEntry);
                return;
            }
        }
    }
};



class FileSystem {
private:
    Directory m_root;
    Directory *m_currentDirectory;

public:
    FileSystem() : m_root(Directory {".", nullptr})
    {
        m_currentDirectory = &m_root;
    }

    ~FileSystem()
    {
    }

private:
    void ls() const
    {
        m_currentDirectory->listEntries();
    }

    void mkdir(const std::string &name) const
    {
        m_currentDirectory->makeDirectory(name);
    }

    void mkfile(const std::string &name) const
    {
        m_currentDirectory->makeFile(name);
    }

    void dir() const
    {
        ls();
    }

    void cd(const std::string &name)
    {
        m_currentDirectory = m_currentDirectory->changeDirectory(name);
    }

    void del(const std::string &name, EntryType type = EntryType::ANY_TYPE) const
    {
        m_currentDirectory->deleteEntry(name, type);
    }

    void rmdir(const std::string &name) const
    {
        del(name, EntryType::DIRECTORY);
    }

    void processCommand(const std::string &command, const std::string &option)
    {
        if (command == "ls")
            return ls();
        else if (command == "dir")
            return dir();
        else if (command == "mkdir")
            return mkdir(option);
        else if (command == "mkfile")
            return mkfile(option);
        else if (command == "cd")
            return cd(option);
        else if (command == "rmdir")
            return rmdir(option);
        else if (command == "del")
            return del(option);
    }

public:
    void readInput(const std::string &fileName)
    {
        std::ifstream infile {fileName};

        std::string line;
        while (std::getline(infile, line)) {
            std::istringstream iss(line);

            std::string command;
            std::string option;

            iss >> command;
            iss >> option;

            std::cout << command << " " << option << "\n";
            processCommand(command, option);
        }
    }
};




int main() {
    FileSystem filesystem;

    filesystem.readInput("path to the input file");

    return 0;
}

Answer 1

Why do the getters for Entry return const references? getEntryType, in particular, does not benefit at all from returning a reference since it just returns an enum value (which is the same size or smaller than what would be returned for the reference).

getParentDirectory should be a const function.

If the first three members of Entry are const members, why is m_parent not const (Directory * const m_parentDirectory)?

In the Directory class, if list would return the number of entries found, that would avoid the need to call count (and iterate thru the directory twice) in the listEntries function. list can be a const function, and the for loop should use const auto &entry.

countEntries and checkEntry can also be const functions.

Why does countEntries return an int? You can't have negative entries in a directory, and if you use std::size_t then you won't need to do the casting with the return value, and just return std::count_if directly. Additionally, countEntries won't work if passed an EntryType of ANY_TYPE.

checkEntry can be simplified a bit by removing the if and separate returns and replacing them with return findEntry != entries.end();, possibly replacing the findEntry value with the find_if call. (findEntry is misnamed; foundEntry would be better.)

Nether makeDirectory or makeFile return or report any sort of error if the entry already exists.

The uses of this-> in listDirectory and changeDirectory are unnecessary. That can also both be const functions.

deleteEntry and checkEntry both use the same find_if. This could be moved into a separate function, which can then be used by changeDirectory as well.

When you make the default root entry in FileSystem, you explicitly create an entry for ".", which none of the subdirectories have. Is this necessary? This will also allow someone to "rmdir ." from the root.

Your "del" command can be used to delete a directory, which is probably not intended since you have an explicit command for that. You can also remove a non-empty directory.

With a little bit of adjustment, you can create a table with commands and function pointers for processCommand to use, rather than using nested if/elses.

You might want to look over this question on Stack Overflow about whether to pass strings by value or reference.

Answer 2

I know a directory looks like a tree structure. But you don't need to implement it as a tree. To me a file system is simply a map from name to object.

class FileSystem
{
    std::map<std::string, File>.   fs;
    std::string                    currentDir;
};

The artificial construct of directories is just a convenient way to simplify things for the human brain. But you don't need to model it like that in memory.

I would use a simple map of 'full path name' to object. Then allow the user to use absolute or relative path names to iterate over the file system object.

This will simplify the number of objects you are maintaining. Your methods on the FileSystem class then internally impose the directory structure on the strings in the map. e.g.:

iterator FileSystem::listbegin(std::string const& directory)
{
    std::directory absDirectory = currentDirectory;
    if (directory[0] == '/') {
        absDirectory = directory;
    }
    else {
        absDirectory += '/' + directory;
    }
    if (absDirectory.back() != '/') {
        absDirectory += '/';
    }

    auto find = std::find_if(std::begin(fs), std::end(fs),
                             [&absDirecotry](auto const& v){return v.first.substr(0, absDirecotry.size()) == absDirecotry;});
    return iterator(find);
}

---

The other thing I would do is that once you have a reference to a file, you should be able to get a standard C++ stream to the object.

std::fsstream      memoryFile(fs, "/ThisIsAfile");

I would want to treat any file object in this memory filesystem just like a file that is in the standard filesystem.

---

This is more like how S3 implements its filesystem structure.