3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Have you removed The code from github? Can you please share it with me? Thank you \$\endgroup\$ – lonely_student Dec 17 '17 at 18:35
  • \$\begingroup\$ @lonely_student Hi, sorry for that. When I got my GitHub subscription I marked most of my repos as private - fixed now. Although the project is long abandoned and I do not remember in what state I left it in. But I guess you'll find out ;) Btw, it's in the dev branch. \$\endgroup\$ – Areuz Jan 5 '18 at 22:57
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Yeah, for me, coming up with descriptive names is the hardest part of coding, I hope it will improve with more practice. I will address the lack of information in the header - I was going to do that anyway, just wanted to get the naming scheme in shape. Yeah, I will switch to copying the metadata instead I guess, I just thought it will be faster - though I DO have to return copies from my functions. I did not notice the Hidden Behaviour, that was not intentional, thanks for noticing! \$\endgroup\$ – Areuz Dec 16 '16 at 22:55
  • \$\begingroup\$ I have rewritten the whole class, stripped a lot of function, but I think now the concept is a little better. I won't repost it here, but I will always be on my GitHub. Anyways, thanks for the tips. \$\endgroup\$ – Areuz Dec 16 '16 at 23:58
  • \$\begingroup\$ @Areuz hope it helped. You may find that you get more feedback over the next week or so, reviews on the site tend to take a bit longer to show up than answers on stack overflow. \$\endgroup\$ – forsvarir Dec 17 '16 at 0:07

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.