Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Modified code

Modified code

#Modified code

Modified code

Formatting problem
Source Link
Toby Speight
  • 87.7k
  • 14
  • 104
  • 325

Minor points:

Minor points

Minor points:

Source Link
Toby Speight
  • 87.7k
  • 14
  • 104
  • 325

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:[email protected]");
 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';
}
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /