What can you recommend me?
The code you have is quite good already, but the most important thing you can do to improve it is to make use of standard library algorithms.
These days, it is considered bad practice to write for loops. It probably blows your mind to hear that, but it’s true. For just a sampling of the guidelines in the C++ core guidelines that explicitly or implicitly imply that, see here, here, here, here, here, here… and there are probably many more.
A loop in the middle of a function is usually a sign that the function is over-engineered, and probably buggy. That’s actually the case for your function, but we’ll get to that.
You should always try to break loops out into their own functions, because they are almost always algorithms in their own right. Better yet, use an existing function that was written by experts and has already been extensively tested. For example, the algorithms in your standard library.
Let’s take a look at your function:
std::string replaceStr(std::string inputStr, char whichOne, char replaceWith, int lastPos)
{
int unitCounter = 0;
for (int Iterator = 0; Iterator < inputStr.length(); Iterator = Iterator + 1)
if (inputStr[Iterator] == whichOne) unitCounter = unitCounter + 1;
for (int Iterator = 0; Iterator < inputStr.length(); Iterator = Iterator + 1)
if (inputStr[Iterator] == whichOne && unitCounter-- != lastPos) inputStr[Iterator] = replaceWith;
return inputStr;
}
This function does two things:
- The first part just counts the occurrences of the character
whichOne.
- The second part is more complicated because it is doing several things at once, but basically, it replaces all instances of
whichOne except the last lastPos few.
Let’s focus on part 1 for now. It just counts the occurrences of the character whichOne. That’s just the standard library algorithm std::count().
Which means your function is basically:
std::string replaceStr(std::string inputStr, char whichOne, char replaceWith, int lastPos)
{
auto unitCounter = std::count(inputStr.begin(), inputStr().end(), whichOne);
for (int Iterator = 0; Iterator < inputStr.length(); Iterator = Iterator + 1)
if (inputStr[Iterator] == whichOne && unitCounter-- != lastPos) inputStr[Iterator] = replaceWith;
return inputStr;
}
Just doing that has made your function 3 lines shorter, and already fixed 1 bug. std::count() is at least as fast as your hand-rolled loop, but may be faster.
(Note that if you’re using C++20, you can also use std::ranges::count(inputStr, whichOne); which is shorter, and safer.)
That’s all for the overview… now for the actual code review.
Code review
#include <iostream>
#include <filesystem>
I see std::string in the code, but I don’t see #include <string> here. Your code “works” because one or both of <iostream> and <filesystem> happen to transitively include <string>… but you shouldn’t rely on this. Include the headers for everything you actually use.
std::string replaceStr(std::string inputStr, char whichOne, char replaceWith, int lastPos)
lastPos isn’t a great name for the last parameter, because it isn’t a position. It’s the number of trailing instances of whichOne to leave unchanged.
for (int Iterator = 0; Iterator < inputStr.length(); Iterator = Iterator + 1)
if (inputStr[Iterator] == whichOne) unitCounter = unitCounter + 1;
There are a number of problems with this loop.
First the stylistic issues:
Iterator is not a great name for a loop variable. Iterator is far more likely to be a type name in a template function than a variable name. This particular loop variable isn’t actually an iterator in any case, it’s an index.
I don’t see a problem with eliding the braces if your loop is a single, simple statement (though other people have religious objections to it). The problem here is that your loop is not a single, simple statement… it’s an if block. It only looks like a single, simple statement because you’ve packed it all into one line. Which brings us to the next point….
Don’t pack multiple things onto a single line. It makes your code harder to read, and much easier to hide bugs.
Don’t do x = x + 1 when ++x will do (and yes, ++x is always better than x++ unless you actually need the latter). Doing it the long way is not only longer—thus making your code noisier and harder to read—it raises red flags for experienced C++ coders, because they will want to do know why you’re doing it the hard way. Doing things that will confuse experienced C++ coders is a great way to hide bugs in your code.
The big issue here, though, is the bug.
Iterator is an int… but inputStr.length() is not an int. It is not only unsigned, it may also be significantly larger than an int (and I believe that is the case on Windows). So when you do Iterator < inputStr.length(), you are comparing mismatched types. If you were compiling with warnings turned on… and you should always compile with all warnings turned on… you would have seen warnings about this.
If you want to do this correctly, you need to get the right type for Iterator. For that, you need to do decltype(inputStr.length()):
for (decltype(inputStr.length()) index = 0; index < inputStr.length(); ++index)
{
if (inputStr[index] == whichOne)
++unitCounter;
}
But this is still terrible in modern of C++. The first step to improvement would be to use a range-for:
for (auto&& c : inputStr)
{
if (c == whichOne)
++unitCounter;
}
But, as mentioned above, much better is to use an actual algorithm. In this case std::count().
for (int Iterator = 0; Iterator < inputStr.length(); Iterator = Iterator + 1)
if (inputStr[Iterator] == whichOne && unitCounter-- != lastPos) inputStr[Iterator] = replaceWith;
This has all the same problems as the previous loop, so I won’t repeat them. I will point out that this is actually a std::replace_if() algorithm:
std::replace_if(inputStr.begin(), inputStr.end(),
[&unitCounter, whichOne, lastPos] (auto c)
{ return c == whichOne and unitCounter-- != lastPos; },
replaceWith).
But it’s a bad idea to keep decrementing even after you’ve reached the limit, because it could theoretically wrap around.
Now, onto main().
std::filesystem::path Path = "D:\\Libraries\\TV Series\\Snowpiercer\\Season 2";
This could be const.
You should also consider alternative ways of writing Windows paths, because using \\ everywhere is ugly and error-prone.
std::filesystem::path works with forward slashes, so you could write "D:/Libraries/TV Series/Snowpiercer/Season 2".
You could also use raw strings: R"(D:\Libraries\TV Series\Snowpiercer\Season 2)".
for (const auto& dirEntry : std::filesystem::recursive_directory_iterator(Path))
{
const std::string File{ dirEntry.path().filename().string() };
std::filesystem::rename(Path / File, Path / replaceStr(File, '.', ' ', 1));
}
Are you… sure you’ve really thought this through? Are you sure you don’t mean to use a non-recursive directory iterator? Or maybe you’re just handling the path incorrectly.
Consider what you’re doing here. Suppose you are currently at the directory entry for D:/Libraries/TV Series/Snowpiercer/Season 2/subdir/file.name.ext. That means File will be file.name.ext. So the rename function is renaming:
- From:
D:/Libraries/TV Series/Snowpiercer/Season 2 / file.name.ext
- To:
D:/Libraries/TV Series/Snowpiercer/Season 2 / file name.ext
But… what happened to subdir?
There’s another issue here, and that is that you are modifying the directory tree while traversing it at the same time.
What you really want to do is just iterate through the filesystem, collecting the paths that need to be changed, and then go through and change them all. Something more like:
// Note: Untested, and not as efficient as it could be. Should also be
// rewritten with proper algorithms. This is just to illustrate the idea.
auto files_to_rename = std::vector<std::tuple<std::filesystem::path, std::filesystem::path>>{};
for (auto&& d : std::filesystem::recursive_directory_iterator(Path))
{
auto const old_filename = d.path().filename().string();
auto const new_filename = replaceStr(old_filename, '.', ' ', 1);
if (old_filename != new_filename)
{
auto old_path = d.path();
auto new_path = d.path();
new_path.replace_filename(new_filename);
files_to_rename.emplace_back(std::move(old_path), std::move(new_path));
}
}
for (auto&& [old_path, new_path] : files_to_rename)
std::filesystem::rename(old_path, new_path);
Algorithmic improvements
Let’s look at your algorithm again:
std::string replaceStr(std::string inputStr, char whichOne, char replaceWith, int lastPos)
{
int unitCounter = 0;
for (int Iterator = 0; Iterator < inputStr.length(); Iterator = Iterator + 1)
if (inputStr[Iterator] == whichOne) unitCounter = unitCounter + 1;
for (int Iterator = 0; Iterator < inputStr.length(); Iterator = Iterator + 1)
if (inputStr[Iterator] == whichOne && unitCounter-- != lastPos) inputStr[Iterator] = replaceWith;
return inputStr;
}
Your algorithm makes two passes over the string: one to count, and one to replace.
Can it be done in one?
Yes. Yes it can.
The trick is to step back and look at the problem in a different way. Instead of moving forwards through the string… let’s move backwards.
If we start at the beginning of the string, the algorithm is pretty complicated, because we want to replace every whichOne character except the last lastPos. In order to do that, we first have to know how many whichOne characters there are, so we know when to stop.
But if we start at the end of the string, the situation is completely different. What we want do now is skip lastPos occurrences of whichOne… and only then start replacing. This is much easier to do:
// Note: Untested, buggy, and ugly. Just for illustration.
std::string replaceStr(std::string s, char c_old, char c_new, int n)
{
// start at the end
auto p = s.data() + s.size();
// skip the last n occurences
for (; p != s.data() and n >= 0; --p)
{
if (*p == c_old)
--n;
}
// now one of two things is true:
// * either p is at the beginning of the string (so we're done)
// * or p is pointing to the first character we need to replace
// do the replacing
for (; p != s.data(); --p)
{
if (*p == c_old)
*p = c_new;
}
return s;
}
But all of the above would be much better written as an algorithm.
// Note: Untested.
std::string replaceStr(std::string s, char c_old, char c_new, int n)
{
std::replace_if(
// Using rbegin and rend moves us *BACKWARDS* through the string
s.rbegin(), s.rend(),
// Only replace if this lambda returns true.
[c_old, &n](auto c) {
if (c == c_old)
{
// Should we skip this occurrence?
if (n > 0)
{
// Yes, so decrement the counter and return false.
--n;
return false;
}
return true;
}
},
c_new);
return s;
}
That’s one pass through the string, only doing replacements if:
- the original character is the one you wan to replace; and
- you have skipped
n instances of the original character already.
One more important thing: Test your code
This is the most important thing you can do as a programmer, in any language. Untested code is garbage code; I will not only refuse to use it in any project I am doing, I won’t even bother to look at it.
Testing your code properly is an art and a science. The first thing you want to do is use a proper testing library. My favourite is Boost.Test, but while that is super powerful, it is also super complicated to use. A lot of people like GoogleTest but… meh. You could also check out Catch2.
Once you have a testing library ready, you should write the tests before the code. There are a number of reasons to do this, not least being that it forces you to write proper, testable interfaces. If you try to write the code first, you can often end up with untestable code… and that’s bad.
So, using Catch2, for your function, you might write:
TEST_CASE("empty string works")
{
REQUIRE(replaceStr("", '.', ' ', 0) == "");
}
TEST_CASE("simple substitution works")
{
auto const data = GENERATE(table<std::string, char, char, std::string>({
// original string, old char, new char, result
{"a.b.c", '.', ' ', "a b c"},
{"1?2?", '?', 'x', "1x2x"},
{"*", '*', '+', "+"},
{"**", '*', '+', "++"},
// and so on, with any other cases you think may be problematic
}));
REQUIRE(replaceStr(std::get<0>(data), std::get<1>(data), std::get<2>(data), 0)
== std::get<3>(data));
}
TEST_CASE("no substitution happens if old char is not in string")
{
auto const data = GENERATE(table<std::string, char, char>({
// original string, old char, new char
{"a.b.c", ' ', '.'},
{"1?2?", 'x', '?'},
{"", '*', '_'},
// and so on...
}));
REQUIRE(replaceStr(std::get<0>(data), std::get<1>(data), std::get<2>(data), 0)
== std::get<0>(data));
}
// also more tests to take lastPos into account...
Once you have these tests, then you write the function… and keep working on it until all the tests pass.
I [am doing something] and can not do one by onemean? \$\endgroup\$