C++ Dirent.h "Wrapper"

Question

I do not want to call this a wrapper, but no other name comes to mind right now, so I'll use it incorrectly for the rest of the question - It's just a class that adds C++ functions to dirent.h but it's very specific to my use case.

All of the code works well, but I've never worked with the dirent.h library and jumping right into a C++ wrapper is something, so I'm really insecure about it's performance and memory usage in practical scenarios.

~But, of course please also judge the readability - I want to know if I should add comments or if my naming scheme and layout is self explanatory.

Quick Description

I'm working on C++ based file manager that needs functions that return vectors of whole directories, of course, that it can display it in one loop. It will also need some more info (metadata I guess), like it's type so it can do all the fancy color syntax.

Here is the final version so far (with some debug code):

inode.h

#ifndef INODECLASS
#define INODECLASS

#include <dirent.h>
#include <string.h>

#include <iostream>

#include <vector>
#include <string>

class Inode 
{
public:
  struct InodeMeta {
    std::string name;
    std::string path;
    unsigned char type;
  };

private:
  std::string pwd_path_; 
  std::vector<InodeMeta *> buffer_;

public:
  Inode(const std::string &);
  ~Inode();

  void ClearBuffer();
  bool ReloadBuffer(const std::string &);

  std::vector<InodeMeta> DumpBuffer(const std::string &);
};

#endif

inode.cc

#include "inode.h"

Inode::Inode(const std::string &arg_path = ".") {
  if ( !ReloadBuffer(arg_path) )
    ClearBuffer();
}

Inode::~Inode() {
  ClearBuffer();
}

void Inode::ClearBuffer() {
  for (auto &item : buffer_)
    delete item;
  buffer_.clear();
}

bool Inode::ReloadBuffer(const std::string &arg_path = "") {
  if (arg_path.length() != 0) pwd_path_ = arg_path;
  DIR *dirstream;
  dirent *diritem;

  ClearBuffer();

  if ( (dirstream = opendir(pwd_path_.c_str())) != NULL )
  {
    while ( (diritem = readdir(dirstream)) != NULL ) {
      if ( strcmp(diritem->d_name, ".") != 0 &&
           strcmp(diritem->d_name, "..") != 0 )

        buffer_.emplace_back( new InodeMeta{ std::string(diritem->d_name),
          std::string(pwd_path_) + "/" + std::string(diritem->d_name),
      diritem->d_type} );
    }
    closedir(dirstream);
  }

  else return false;
  return true;
}

std::vector<Inode::InodeMeta> Inode::DumpBuffer(const std::string &arg_path = "") {
  if (arg_path.length() != 0) ReloadBuffer(arg_path);

  std::vector<InodeMeta> ret_list;

  for (auto &item : buffer_)
    ret_list.push_back(*item);

  return ret_list;
}

//DEBUG ONLY//

#include <iostream>

int main() {
  Inode I;

  std::vector<Inode::InodeMeta> inodelist = I.DumpBuffer();

  for (auto &item : inodelist)
    std::cout << item.name << " ; " << item.path << '\n';

  I.ReloadBuffer("/tmp/");
  inodelist.clear();
  inodelist = I.DumpBuffer();

  for (auto &item : inodelist)
    std::cout << item.name << " ; " << item.path << '\n';
}

Just in case you either want to see the whole project (I started it like 3 days ago, don't judge me too hard), or just make a quick git clone and test it yourself here.

Answer 1

Parameter names

This is one of my pet peeves:

Inode(const std::string &);

The point of a header file / class definition at least in my head is to separate the interface to your code from the implementation of your code. Not putting parameter names in your function declarations makes your interface incomplete. Sure, you know what the parameter is, but your wrote the code. From reading your class header I don't have a clue what the string might represent, there's no comments to tell me, no name, so I'm either relying on your documentation (?) or have to look at the implementation to figure it out.

Default parameters

Default parameters only effect code that can see them. You've defined default parameters for your methods, but only in your implementation file. I find that a little odd. If it's a good default, it should be in the header.

Naming

Descriptive naming is important and significantly reduces the learning time for people new to your code. I had to look Inode up to see what it was. It probably makes sense if you have experience in that area, but as an implementation of a folder structure it's not the first name I'd be looking for. pwd to me means 'print working directory', so pwd_path_ seems like an odd name. Perhaps wd_path would be better. buffer_ is very non-descript. It's a list of metadata, why not call it that?

You have a similar issue with your method naming. DumpBuffer doesn't really say 'read the directory and return the meta data to me`.

Blank Lines

Some of your blank lines don't really make sense to me, they make your code harder to read. Consider this:

if ( strcmp(diritem->d_name, ".") != 0 &&
           strcmp(diritem->d_name, "..") != 0 )

        buffer_.emplace_back( new InodeMeta{ std::string(diritem->d_name),
          std::string(pwd_path_) + "/" + std::string(diritem->d_name),
      diritem->d_type} );

You set up an unbraced if statement and then leave a blank line between the if and the code that belongs to it. This is an invitation to be misinterpreted. I feel much the same about the else condition at the end of the function:

}

  else return false;

You close the if, leave a blank line then put the else. It makes the else feel detached.

Pointers

What are you getting from storing pointers in your buffer_, rather than instances of InodeMeta. Since you return copies anyway from your DumpBuffer method, it feels like you've just added extra handling requirements for little to no benefit? If you want to save duplication then storing and returning a shared_ptr instead might be more efficient.

Hidden Behaviour

I'm not sure that having DumpBuffer behave differently depending on whether or not you pass it an empty string is a great idea. If I pass in "/someFolder" then the folder is read. If I pass it in again, "/someFolder", then the folder is read again (so any new files would be noticed for example). If instead for the second call I had passed in "" then the original folder contents would have been returned. If this is desired behaviour, then I'd at least put a comment in the header / documentation because it's not what I would expect.