2
\$\begingroup\$

I am trying to come up with a good File System Design using C++ / OOP. This is being done to gain an understanding of System Design and Design Patterns.

I have some basic functionality, like reading and writing files, adding and deleting files and folders, calculating the size of files and folders. The design pattern I am trying to use is Composite Design pattern.

My design uses a base class called BaseComponent which contains facilities common to both files and folders. The class Folder deals with adding and deleting of files and folders while class File deals with reading and writing of files. Both File and Folder are derived from BaseComponent.

Here is the code:

#include <iostream>
#include <string>
#include <set>

class BaseComponent
{
    std::string name_v;
    BaseComponent* parent_v;
    bool is_folder;
public:
    BaseComponent(std::string name, BaseComponent* parent = nullptr, bool f = true) : name_v{name}, parent_v{parent}, is_folder{f}
    {
    }

    virtual ~BaseComponent()
    {

    }

    void change_name(std::string& name)
    {
        name_v = name;
    }

    std::string name() const
    {
        return name_v;
    }

    BaseComponent* parent() const
    {
        return parent_v;
    }

    void del()
    {
        if(parent_v == nullptr)
        {
            return;
        }
        parent_v -> delete_component(this);
    }
    virtual int size() const
    {
        return 0;
    }
    virtual void add_component(BaseComponent*)
    {

    }
    virtual void delete_component(BaseComponent*)
    {

    }
};

class Folder : public BaseComponent
{
    std::set<BaseComponent*> children;
public:
    Folder(std::string name, Folder* parent = nullptr, bool is_folder = true) : BaseComponent{name, parent, is_folder}
    {

    }

    ~Folder()
    {

    }

    int size() const
    {
        int size_v = 0;
        for(auto child : children)
        {
            size_v += child -> size();
        }
        return size_v;
    }

    int num_ff()
    {
        return children.size();
    }

    void add_component(BaseComponent* b)
    {
        children.insert(b);
    }

    void delete_component(BaseComponent* b)
    {
        children.erase(b);
    }
};

class File : public BaseComponent
{
    std::string contents_v;
public:
    File(std::string contents, std::string name, Folder* parent, bool is_folder = false) : BaseComponent{name, parent, is_folder}, contents_v{contents}
    {
    }

    ~File()
    {

    }
    int size() const
    {
        return sizeof(this);
    }

    void write(std::string content)
    {
        contents_v = content;
    }

    std::string read() const
    {
        return contents_v;
    }

};

/*class FileSystem
{

};*/

int main()
{
    Folder root{"/"};
    Folder home{"home", &root};
    root.add_component(&home);
    Folder lib{"lib", &root};
    root.add_component(&lib);
    Folder dev{"dev", &root};
    root.add_component(&dev);
    std::cout << "No. of components in root: " << root.num_ff() << "\n";
    std::cout << "No. of components in home: " << home.num_ff() << "\n";
    File test{"x = 0, y = 0", "Configuration.txt", &root};
    root.add_component(&test);
    Folder ws{"ws", &home};
    home.add_component(&ws);
    File fs{"fs file", "fs.cpp", &ws};
    ws.add_component(&fs);
    File git{"user: dummy", "git_config", &home};
    home.add_component(&git);
    std::cout << "Contents of git: " << git.read() << "\n";
    git.write("user: new\n email:new@new.com");
    std::cout << "Contents of git: " << git.read() << "\n";
    std::cout << "No. of components in root: " << root.num_ff() << "\n";
    std::cout << "No. of components in home: " << home.num_ff() << "\n";
    std::cout << "No. of components in ws: " << ws.num_ff() << "\n";
    dev.del();
    std::cout << "No. of components in root: " << root.num_ff() << "\n";
    std::cout << "Size of git: " << git.size() << "\n";
    std::cout << "Size of root: " << root.size() << "\n";
    return 0;
}
  1. The add_component is being called from main. How can I change that? I don't mind calling it inside a new class called FileSystem by creating functions such as mkdir or mkfile to handle folder and file creation. But I am not able to come up with a good design.

  2. If FileSystem is being implemented, I would like to avoid having to take the pointer to parent inside the main, while creating files and folders. This is again something that is stopping me from coming up with a better design.

  3. How to improve the functionalities implemented? Are there any significant issues in this part?

  4. Is a better design possible without changing classes and their interface?

  5. Kindly provide an overall review.

\$\endgroup\$
  • 1
    \$\begingroup\$ Have you looked at how (proposed) std::filesystem represents these, or is this a completely independent approach? \$\endgroup\$ – Toby Speight Sep 27 '18 at 10:48
  • \$\begingroup\$ @TobySpeight I haven't looked into std::filesystem as I didn't know that it existed. However, my objective is to learn system design. So I need to implement it anyway. \$\endgroup\$ – skr_robo Sep 27 '18 at 11:01
2
\$\begingroup\$

Something that jumps out immediately is the is_folder member. It's a bad sign if a base class needs to know what subclass it is. Even more concerning, the subclass constructors allow one to create a File with is_folder set, or a Folder with is_folder reset. Thankfully, this member seems never to be used, so it can be omitted altogether.


I think that BaseComponent ought to be an abstract class. A simple way to do so is to make size() a pure virtual method:

virtual std::size_t size() const = 0;

(I've used a more appropriate return type, too - though some platforms support files larger than addressable memory, so be careful).


Similarly, the destructor that's declared just to make it virtual (well done for remembering) can be defined =default:

virtual ~BaseComponent() = default;

The subclass destructors can be omitted, as the compiler-generated ones will be exactly equivalent.


We can save the two-step construct-then-add by having the constructor also add to the parent entry:

BaseComponent::BaseComponent(std::string name, Folder* parent)
    : name_v{std::move(name)},
      parent_v{parent}
{
    if (parent) {
        parent->add_component(this);
    }
}

It also makes sense for a destructor to remove the object from its parent (otherwise we could end up with dangling references), and for a deleted Folder to null out its children's parent pointers.


We can help avoid mistakes by using the override keyword when we reimplement size() in the subclasses:

std::size_t size() const override
{
    return sizeof this;
}

BTW, I hope that sizeof this is just a placeholder for something more useful later (all pointers to File objects will have the same size on any given system). You probably meant return contents_v.size(); there.


Minor points:

  • num_ff() should be const
  • Constructor arguments passed by value should be std::move()d to their new homes.
  • BaseComponent constructor and destructor can be protected. Consider suppressing copy construction and copy assignment.
  • BaseComponent isn't a very informative name.

Modified code

#include <numeric>
#include <string>
#include <set>

class Folder;
class File;

class FileSystemObject
{
    std::string name_v;
    Folder* parent_v;

protected:
    FileSystemObject(std::string name, Folder* parent);

    FileSystemObject(const FileSystemObject&) = delete;
    FileSystemObject& operator=(const FileSystemObject&) = delete;

    virtual ~FileSystemObject();

public:
    void change_name(std::string& name)
    {
        name_v = name;
    }

    std::string name() const
    {
        return name_v;
    }

    Folder* parent() const
    {
        return parent_v;
    }

    virtual std::size_t size() const = 0;

    void set_parent(Folder* parent);
};


class Folder : public FileSystemObject
{
    std::set<FileSystemObject*> children = {};
public:
    Folder(std::string name, Folder* parent = nullptr)
        : FileSystemObject{std::move(name), parent}
    {
    }

    ~Folder()
    {
        for (auto c: children) {
            c->set_parent(nullptr);
        }
    }

    std::size_t size() const override
    {
        return std::accumulate(children.begin(), children.end(), 0,
                               [](auto sum, auto child){ return sum + child->size(); });
    }

    int num_ff() const
    {
        return children.size();
    }

    void add_component(FileSystemObject* b)
    {
        children.insert(b);
    }

    void delete_component(FileSystemObject* b)
    {
        children.erase(b);
    }
};


class File : public FileSystemObject
{
    std::string contents_v;
public:
    File(std::string contents, std::string name, Folder* parent)
        : FileSystemObject{name, parent},
          contents_v{contents}
    {
    }

    std::size_t size() const override
    {
        return contents_v.size();
    }

    void write(std::string content)
    {
        contents_v = std::move(content);
    }

    std::string read() const
    {
        return contents_v;
    }

};

FileSystemObject::FileSystemObject(std::string name, Folder* parent)
    : name_v{std::move(name)},
      parent_v{parent}
{
    if (parent) {
        parent->add_component(this);
    }
}


FileSystemObject::~FileSystemObject()
{
    if (parent_v) {
        parent_v->delete_component(this);
    }
}

void FileSystemObject::set_parent(Folder* parent)
{
    if (parent_v) {
        parent_v->delete_component(this);
    }
    if ((parent_v = parent)) {    // assignment!
        parent_v->add_component(this);
    }
}


#include <iostream>

int main()
{
    Folder root{"/"};
    Folder home{"home", &root};
    Folder lib{"lib", &root};
    Folder dev{"dev", &root};
    std::cout << "No. of components in root: " << root.num_ff() << '\n';
    std::cout << "No. of components in home: " << home.num_ff() << '\n';
    File test{"x = 0, y = 0", "Configuration.txt", &root};
    Folder ws{"ws", &home};
    File fs{"fs file", "fs.cpp", &ws};
    File git{"user: dummy", "git_config", &home};
    std::cout << "Contents of git: " << git.read() << '\n';
    git.write("user: new\n email:new@new.com");
    std::cout << "Contents of git: " << git.read() << '\n';
    std::cout << "No. of components in root: " << root.num_ff() << '\n';
    std::cout << "No. of components in home: " << home.num_ff() << '\n';
    std::cout << "No. of components in ws: " << ws.num_ff() << '\n';
    dev.set_parent(nullptr);
    std::cout << "No. of components in root: " << root.num_ff() << '\n';
    std::cout << "Size of git: " << git.size() << '\n';
    std::cout << "Size of root: " << root.size() << '\n';
}
\$\endgroup\$
  • \$\begingroup\$ Thank you so much for the review (upvoted). I tried using add_component inside the constructor similar to what you have shown, but I missed the check for nullptr. I thought the problem was because the base class was calling derived class member inside constructor even though parent was already constructed. \$\endgroup\$ – skr_robo Sep 27 '18 at 14:40
  • 1
    \$\begingroup\$ It's also important to note the forward declaration of class Folder so we can use Folder* for members and parameters, and moving the definition of the constructor and destructor to after Folder, so we can use its methods. \$\endgroup\$ – Toby Speight Sep 27 '18 at 14:43

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.