I wanted to have some advice about the code I have.
I managed to get what I wanted done, but I do not think it is the "proper" way of doing it in the programmers' world.
Could you help me improve the code by any means and also if there are any better ways of doing this please share them as well.
I have files named in the format:
501.236.pcd
501.372.pcd
...
612.248.pcd etc.
I wanted to put the filenames in ascending order according to the filenames using C++.
This is the code I use:
#include <string>
#include <iostream>
#include <boost/filesystem.hpp>
#include <sstream>
using namespace std;
using namespace boost::filesystem;
int main()
{
vector <string> str,parsed_str;
path p("./fake_pcd");
string delimiter = ".";
string token,parsed_filename;
size_t pos = 0;
int int_filename;
vector <int> int_dir;
//insert filenames in the directory to a string vector
for (auto i = directory_iterator(p); i != directory_iterator(); i++)
{
if (!is_directory(i->path())) //we eliminate directories in a list
{
str.insert(str.end(),i->path().filename().string());
}
else
continue;
}
//parse each string element in the vector, split from each delimiter
//add each token together and convert to integer
//put inside a integer vector
parsed_str = str;
for (std::vector<string>::iterator i=parsed_str.begin(); i != parsed_str.end(); ++i)
{
cout << *i << endl;
while ((pos = i->find(delimiter)) != string::npos) {
token = i->substr(0,pos);
parsed_filename += token;
i->erase(0, pos + delimiter.length());
}
int_filename = stoi(parsed_filename);
int_dir.push_back(int_filename);
parsed_filename = "";
}
cout << endl;
parsed_str.clear();
sort(int_dir.begin(), int_dir.end());
//print the sorted integers
for(vector<int>::const_iterator i=int_dir.begin(); i != int_dir.end(); i++) {
cout << *i << endl;
}
//convert sorted integers to string and put them back into string vector
for (auto &x : int_dir) {
stringstream ss;
ss << x;
string y;
ss >> y;
parsed_str.push_back(y);
}
cout << endl;
//change the strings so that they are like the original filenames
for(vector<string>::iterator i=parsed_str.begin(); i != parsed_str.end(); i++) {
*i = i->substr(0,3) + "." + i->substr(3,3) + ".pcd";
cout << *i << endl;
}
}
This is the output, first part is in the order the directory_iterator gets it, the second part is the filenames sorted in integers, and the last part is where I change the integers back into strings in the original filename format.
612.948.pcd
612.247.pcd
501.567.pcd
501.346.pcd
501.236.pcd
512.567.pcd
613.008.pcd
502.567.pcd
612.237.pcd
612.248.pcd
501236
501346
501567
502567
512567
612237
612247
612248
612948
613008
501.236.pcd
501.346.pcd
501.567.pcd
502.567.pcd
512.567.pcd
612.237.pcd
612.247.pcd
612.248.pcd
612.948.pcd
613.008.pcd
1 Answer 1
If you have access to C++17, you can use std::filesystem
instead of boost::filesystem
.
This is gonna cause a ton of problems by obscuring the source of names used in the code and introducing name clashes:
using namespace std; using namespace boost::filesystem;
Qualify names from std
instead, and maybe namespace fs = std::filesystem;
.
Overall, the main
function is very long and requires a lot of brain energy to understand.
Declaring a lot of variables at the start of a block makes the logic hard to follow:
vector <string> str,parsed_str; path p("./fake_pcd"); string delimiter = "."; string token,parsed_filename; size_t pos = 0; int int_filename; vector <int> int_dir;
Some of the variables have sub-optimal names. Also consider accepting the path as an argument for more flexibility:
int main(int argc, char* argv[])
{
// validate ...
std::path directory{std::string{argv[1]}};
// ...
}
This is convoluted:
//insert filenames in the directory to a string vector for (auto i = directory_iterator(p); i != directory_iterator(); i++) { if (!is_directory(i->path())) //we eliminate directories in a list { str.insert(str.end(),i->path().filename().string()); } else continue; }
Simplification: (note that directory_iterator
is a range by itself)
std::vector<std::string> filenames;
for (const auto& entry : fs::directory_iterator{directory}) {
if (!entry.is_directory()) {
filenames.push_back(entry.path().filename().string());
}
}
Sorry, I gave up trying to understand everything beyond this point.
Here's basically how I would write the same code (not tested):
#include <algorithm>
#include <cstdlib>
#include <filesystem>
#include <iostream>
#include <string>
#include <vector>
namespace fs = std::filesystem;
fs::directory_entry parse_args(int argc, char* argv[])
{
if (argc != 2) {
std::cerr << "Usage: " << argv[0] << " <directory>\n";
std::exit(EXIT_FAILURE);
}
fs::directory_entry directory{argv[1]};
if (!directory.is_directory()) {
std::cerr << '\'' << argv[1] << "' is not a directory\n";
std::exit(EXIT_FAILURE);
}
return directory;
}
int main(int argc, char* argv[])
{
auto directory = parse_args(argc, argv);
std::vector<fs::path> filenames;
for (const auto& entry : fs::directory_iterator{directory}) {
if (entry.is_regular_file()) {
filenames.push_back(entry.path().filename());
}
}
std::sort(filenames.begin(), filenames.end(),
[](const auto& lhs, const auto& rhs) {
return lhs.string() < rhs.string();
});
for (const auto& file : filenames) {
std::cout << file.string() << '\n';
}
}
sort
? \$\endgroup\$std::sort
onstr
. It will sort it according to alphabetical order which might deviate from what you want but this is what is usually used. \$\endgroup\$