Traverse directory and store data

Question

During one of assignment, I need to directory and store into proper data structure. I did following way and create to store as tree with multiple child and child will have linked list. Any improvement or suggestion.

File format:
    /path1/path2/path3
    /path1/path2/path3/cell_lists
    /path1/path2/path3/path4
    /path1/path2/path3/path4/ALLCELLS
    /path1/path2/path3/path4/ALLCELLS/cells
    /path1/path2/path3/path4/ALLCELLS/cells/fild
    /path1/path2/path3/path4/ALLCELLS/cells/fild2

source code

    #include<iostream>       
    #include<fstream>
    #include<vector>
    #include<boost/thread/thread.hpp>
    #include<boost/filesystem.hpp>
    #include "subprocess.h"
    #include "Command.h"
    namespace fs = boost::filesystem;

    void create_dir() {
        fs::path p("/user_path");
        std::ofstream outfile("dir_names_150");
        int maxdepth = 1;
        fs::recursive_directory_iterator end_iter;
        boost::system::error_code dir_error;
        for(fs::recursive_directory_iterator begin(p, dir_error); begin != end_iter; ++begin) {
            try {
                if(fs::is_directory(begin->status())){
                    if(begin.level()== maxdepth)
                        begin.no_push();
                    outfile<<begin->path().string()<<std::endl;
                }
            }catch(fs::filesystem_error &ex) {
                std::cout<<"Boost Filesystem Error"<< ex.what()<<std::endl;
            }
            catch(const std::exception &ex){
                std::cout<<"Standard Exception"<<ex.what()<<std::endl;
            }
        }
        outfile.close();
    }

    std::vector<std::string> read_dir() {
        std::string line;
        std::vector<std::string> dir_names;
        std::ifstream myfile("dir_names_150");
        while(getline(myfile, line)) {
            dir_names.push_back(line);
        }
        myfile.close();
        return dir_names;
    }

    struct Dirinfo {
        uint64_t dir_size;
        std::string owner;
        std::string dir_name;
        std::string dir_path;
        std::string f_time;
        std::vector<Dirinfo*> list;
        Dirinfo *next;
    };

    class Dirmapping {
        private:
            Dirinfo *root;
            std::vector<std::string> dirs;
        public:
            Dirmapping(){}
            Dirmapping(std::vector<std::string> &dir_name);
            void createmap();
            void printmap();
    };

    Dirmapping::Dirmapping(std::vector<std::string> &dirs) {
        this->dirs = dirs;
        root = nullptr;
    }


    void Dirmapping::createmap() {
        for(std::vector<std::string>::iterator it = this->dirs.begin();  it != dirs.end(); ++it) {
            fs::path insert_path(*it);
            Dirinfo *dirinfo = new Dirinfo;
            dirinfo->dir_path = *it;
            dirinfo->dir_name = insert_path.filename();
            dirinfo->f_time = std::to_string(fs::last_write_time(insert_path));
            dirinfo->next = nullptr;
            dirinfo->owner = "";
            dirinfo->dir_size = 0;
            if (root == nullptr) {
                root = dirinfo;
            }
            else {
                Dirinfo *temp = root;
                fs::path compare_path(*it);
                bool found_root = false;
                while(temp->next != nullptr) {
                    fs::path p(temp->dir_path);
                    while (compare_path.string().length()>2){
                        if(fs::equivalent(compare_path, p)){
                            found_root = true;
                            break;
                        }
                        compare_path = compare_path.parent_path();
                    }
                    if(found_root)
                        break;
                    temp= temp->next;
                }
                if(found_root)
                    temp->list.push_back(dirinfo);
                else
                    temp->next = dirinfo;
            }
        }
    }

    void Dirmapping::printmap() {
        Dirinfo *temp = root;
        while(temp != nullptr){
            std::cout<<"Root::"<<temp->dir_path<<std::endl;
            if(temp->list.size()){
                std::cout<<"children"<<std::endl;
                for(std::vector<Dirinfo*>::iterator it = temp->list.begin(); it != temp->list.end(); ++it){
                    std::cout<<"-->"<<(*it)->dir_path<<std::endl;
                }
            }
        }
    }


    int main() {
        create_dir();
        std::vector<std::string> dir_names = read_dir();
        Dirmapping dirmap(dir_names);
        dirmap.createmap();
        dirmap.printmap();
        return 0;
    }

Answer 1

create_dir()

This function's name is misleading. It does not create any directories.

for(fs::recursive_directory_iterator begin(p, dir_error); begin != end_iter; ++begin) {

You are using the error_code version of begin() to create the directory iterator without risking an exception, but then you don't check to see if there was an error. If your intention is to avoid exceptions outside of the loop, you should probably check the error status, and also use the version of increment() that takes an error status:

for (fs::recursive_directory_iterator begin{p, dir_error}; !dir_error && begin != end_iter; begin.increment(dir_error))

Within the loop body, if you're just going to be ignoring the errors - or at most, printing a message about them - consider using the error code pattern there, too. As in:

auto ec = boost::system::error_code{};
auto stat = begin->status(ec);
if (!ec)
{
    if (fs::is_directory(stat))
    {
        if (begin.level() == maxdepth)
            begin.no_push();
        outfile << begin->path().string() << '\n';
    }
}
else
{
    std::cerr << "error: " << ec.message() << '\n';
}

This helps avoid expensive exception machinery when all you want to do is print a message and continue.

Avoid std::endl. In fact, just never use it.

Catch exceptions by const reference (normally) - maybe you just forgot the const when catching the filesystem exception.

outfile.close();

Closing the file stream is unnecessary.

read_dir()

Why do you read all the directory names into a file in create_dir(), then open the file again and read all those same names into a vector in this function? If all you want is the vector of directories, you can do that in create_dir() (which, again, needs a better name, but then so does this function). If you really want to save the list of directories in a file, it's much easier to read them all into a vector, then write the contents of the vector. That way you only have to open the file one time.

std::vector<std::string> read_dir() {
    std::string line;
    std::vector<std::string> dir_names;
    std::ifstream myfile("dir_names_150");
    while(getline(myfile, line)) {
        dir_names.push_back(line);
    }
    myfile.close();
    return dir_names;
}

You can write this pattern more idiomatically like this:

std::vector<std::string> dir_names;
std::ifstream myfile("dir_names_150");
for (auto line = std::string{}; getline(myfile, line); )
    dir_names.push_back(line);
return dir_names;

But that's a matter of taste.

Once again, streams close automatically.

Dirinfo

So this is your linked list node class. I don't really see the point of reimplementing a linked list when you have one in the library: std::list.

uint64_t dir_size;

First, this should be std::uint64_t. You should also include <cstdint>.

But you should avoid fixed-size types because they're not portable. Instead use std::uint_least64_t or std::uint_fast64_t depending on whether you're more concerned about speed or space. Or use a more relevant type (which I can't guess what that might be, because it's only set to 0).

std::string dir_name;
std::string dir_path;

I don't know the purpose of owner, but these seem like they should be fs::path, not std::string. And you don't need both. You just need fs::path dir_path;, and the directory name is just dir_path.filename().

std::string f_time;

It seems misguided to store the last modified time as a string, rather than some kind of time type.

std::vector<Dirinfo*> list;

Normally storing pointers in a vector like this is a red flag. In fact, you have some serious problems because of it.

However, you're kinda stuck because DirInfo is still an incomplete type. You have to store pointers (or references).

In fact, what you have is a mess, because it's not clear anywhere in your program who owns what. Who owns all the Dirinfo items in list? Who owns next? Your program leaks every single Dirinfo you create, and it's not clear where you'd do the cleanup to fix that.

Dirmapping

You should use default initializers, especially for fundamental types (like pointers):

class Dirmapping {
    private:
        Dirinfo *root = nullptr;

This will ensure that root is nullptr even if you forget to make it so (which you do in your default constructor).

Dirmapping(){}

Prefer to write this like:

Dirmapping() = default;

Constructors that take a single parameters should be explicit, so:

Dirmapping(std::vector<std::string> &dir_name);

should be marked explicit. Also, because you don't modify the vector in the function, you should take it by const reference.

void printmap();

This function should be marked const, because it doesn't modify the object.

Dirmapping::Dirmapping(std::vector<std::string> &dirs) {
    this->dirs = dirs;
    root = nullptr;
}

As mentioned before, this should take the vector by const&.

If you use class initializers, you don't need to set root to nullptr - that will be automatic.

Don't access class members like this->. There is no need in most cases.

Prefer to initialize class members like this:

Dirmapping::Dirmapping(std::vector<std::string> const& dirs) :
    dirs{dirs}
{}

Next is createmap():

for(std::vector<std::string>::iterator it = this->dirs.begin();  it != dirs.end(); ++it) {

This is a clunky, old-fashioned way to iterate over a vector.

The first improvement is to use auto:

for (auto it = dirs.begin();  it != dirs.end(); ++it)

Already that's much simpler.

But even better would be to use range-for:

for (auto&& dir : dirs)

or in this case:

for (auto const& dir : dirs)

Even better still would be to use an algorithm that makes it explicit what the loop is doing.

Dirinfo *dirinfo = new Dirinfo;

Using new is almost always a sign that you're doing something wrong. And in fact, you're leaking every one of the Dirinfo objects you create. You never do any deletes... but you shouldn't anyway. You should be using smart pointers:

auto dirinfo = std::make_unique<Dirinfo>{};

That means that root should be:

std::unique_ptr<Dirinfo> root = nullptr;

Dirinfo::next should probably also be a unique_ptr. And Dirinfo::list should maybe be std::vector<std::unique_ptr>. All this depends on who you want to own which objects.

dirinfo->dir_path = *it;
dirinfo->dir_name = insert_path.filename();

This is only necessary because you made dir_path a string. If you made it a path, you wouldn't need dir_name, and this would become:

dirinfo->dir_path = dir;

That also makes insert_path superfluous.

dirinfo->f_time = std::to_string(fs::last_write_time(insert_path));

This is unnecessary if you just store f_time as a std::time_t.

Let's skip down to the loop.

fs::path p(temp->dir_path);

This would be unnecessary if dir_path were a path and not a string.

while (compare_path.string().length()>2){

What I suspect you're trying to do here is test to see if your path is just /. (I'm just guessing, because your code has no comments to explain your intentions.) This is a roundabout way to do it that won't give you the right answer in all cases. Instead, just use path's members. In this case, assuming your paths are all absolute paths, all you'd need to check is:

while (compare_path.has_relative_path())

On to printmap().

if(temp->list.size())

You probably mean:

if (!temp->list.empty())

And finally:

for(std::vector<Dirinfo*>::iterator it = temp->list.begin(); it != temp->list.end(); ++it){
    std::cout<<"-->"<<(*it)->dir_path<<std::endl;
}

Once again, this can be written much more simply with a range-for loop:

for (auto const& item : temp->list)
    std::cout << "-->" << item.dir_path << '\n';

And of course, don't use std::endl.

The biggest problem to fix with your code is that it leaks every Dirinfo object it creates. Use smart pointers, and avoid new altogether. Better yet, don't write your own linked list when there's a perfectly good one in the standard library.

I would also STRONGLY suggest writing some comments in your code to explain what your intentions are.