4
\$\begingroup\$

I am trying to write a function that would do what a cd .. would do. Basically get the parent of the given path. eg:

C:\ --> C:\
C:\Users\xxx --> C:\Users
test1\test2\test4 --> test1\test2\
/var/etc/jpt --> /var/etc

I have tested this in windows and linux. Here are the list of comments I have.

I am declaring forward_slash and backward_slash so that it would work on both win and linux. One might ask, what if there are other path delimeter? For now, lets just stick to these two delimeters. If I do not find any of the two delimiters, I just return the path itself.

I loop over the dir_path trying to find the first and second delimiter.

  • I have to do -1 here int index = dir_path.size()-1; to avoid out of bound error.

  • If there is no delimiter found from first delimiter position (using find_last_of) then I return sub string from 0 to first delimiter position + 1. I have to +1 here because the for loop already moved the index by -1.

  • If there is any thing in between the two consecutive delimiter, I just return the sub string from 0 to second delimiter position.

  • I have to do the if (second_delimeter_pos == std::string::npos) first.

The below code also has test paths.

#include <iostream>
#include <string>
#include <vector>


std::vector<std::string> generatePaths() {
    std::vector<std::string> paths;

    paths.emplace_back("C:\\Users\\xxxx\\\\");
    paths.emplace_back("C:\\");
    paths.emplace_back("C:");
    paths.emplace_back("C:\\\\");
    paths.emplace_back("C://");
    paths.emplace_back("C:/");
    paths.emplace_back("test\\test1\\tmp2");
    paths.emplace_back("C:\\Users\\xxxxx\\projects\\\\\\play_ground\\\\\\");
    paths.emplace_back("C:/Users/xxxxxx/projects/////play_ground////");
    
    return paths;
}

std::string getParentDir(const std::string& dir_path) {

    std::string forward_slash = "/";
    std::string backward_slash = "\\";
   
    std::string delimiter;

    if (dir_path.find(forward_slash) != std::string::npos) {
        delimiter = forward_slash;
    }
    else if (dir_path.find(backward_slash) != std::string::npos) {
        delimiter = backward_slash;
    }
    else {
        return dir_path;
    }
    std::string parent_path;

    for (int index = dir_path.size()-1; index > 0 ; --index) {
        
        int first_delimeter_pos = index;
        size_t second_delimeter_pos = (int)dir_path.find_last_of(delimiter, first_delimeter_pos);
        int diff = first_delimeter_pos - second_delimeter_pos;
        
        if (second_delimeter_pos == std::string::npos) {
            parent_path = dir_path.substr(0, first_delimeter_pos+1);
            break;
        }
        if (diff > 1) {
            parent_path = dir_path.substr(0, second_delimeter_pos);
            break;
        }
        index -= diff;
    }

    return parent_path;
}
int main() {
    std::vector<std::string> test_paths = generatePaths();
    for (int i = 0; i < test_paths.size(); ++i) {
        std::string my_dir = test_paths[i];
        std::string parent_dir = getParentDir(my_dir);

        std::cout << "dir =  " << my_dir << " :: parent dir = " << parent_dir << std::endl;

    }
}

Any suggestion, improvements and pitfall on design and algorithm is highly appreciated.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review. I have rolled back your lates edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Oct 13 '20 at 9:10
5
\$\begingroup\$

If you can use c++17 you can use the stl filesystem, this header contains faculties for performing operations on paths and files and would make your code simpler, for example the delimiter can be handled with the std::filesystem::path class.

According to the documentation of std::filesystem::path a path can be normalized by following this algorithm:

  • If the path is empty, stop (normal form of an empty path is an empty path)
  • Replace each directory-separator (which may consist of multiple slashes) with a single path::preferred_separator.
  • Replace each slash character in the root-name with path::preferred_separator.
  • Remove each dot and any immediately following directory-separator.
  • Remove each non-dot-dot filename immediately followed by a directory-separator and a dot-dot, along with any immediately following directory-separator.
  • If there is root-directory, remove all dot-dots and any directory-separators immediately following them.
  • If the last filename is dot-dot, remove any trailing directory-separator.
  • If the path is empty, add a dot (normal form of ./ is .)

Look here for more info

Unnecessary cast size_t to int
You cast the result of find_last_of to int and assign it to a size_t this while the result of find_last_of is of type size_t.

Prefer using '\n' instead of std::endl
It won't make a lot of difference in this particular case but it is good to make it an habbit, std::endl will flush the output buffer each time what can be a performance hit. See this for more

Use range based for loop
Because you don't need the index for anything else but accessing the item in the vector you can replace:
for (int i = 0; i < test_paths.size(); ++i)

with: for(auto path : test_paths) where path will then be the item at n index of the vector.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I updated the code after you suggestion, converted everything to size_t. I will also look into the std::filesystem::path. \$\endgroup\$ – boka Oct 13 '20 at 8:30
  • \$\begingroup\$ the for loop goes from test_paths.size() to 0 not the other way round. \$\endgroup\$ – boka Oct 13 '20 at 21:58
  • \$\begingroup\$ The one I used as example, that's copied from your source code does not go from test_paths.size() to zero but the other way around. You say int i = 0; i < test_paths.size(); ++i; this will increment i, not decrement. \$\endgroup\$ – Rick Nijhuis Oct 14 '20 at 6:36

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.