Problem
I’m learning regex for c++, and tried to make a simple function. It is very small but I’m satisfied, because this is the first time I managed to pass function as argument. This is a replace function. Please give me tips on how to improve:
std::string Replace(std::string text, std::regex reg, std::function<std::string(std::smatch)> func){
std::smatch matcher;
std::string result = "";
while (std::regex_search(text, matcher, reg))
{
result += matcher.prefix().str();
result += func(matcher);
text = matcher.suffix().str();
}
result += text;
return result;
}
Now here is a test, I pass a javascript like formatted string, with regex \$\{\s*([a-zA-Z0-9]*?)\s*\} and I put my name and age into a variable.
int main(){
std::string name = "Ihsan";
std::string age = "13";
std::string text = "hello my name is ${name}, and im ${age} years old.";
std::regex pattern("\$\{\s*([a-zA-Z0-9]*?)\s*\}");
std::cout << Replace(text, pattern, [name, age](auto matcher) -> std::string{
std::string str1 = matcher.str(1);
if(str1 == "name"){
return name;
}
if(str1 == "age"){
return age;
}
});
}
Output is:
hello my name is Ihsan, and im 13 years old.
Solution
There are a lot easier ways to print your name and age, but I’ll assume you know that 🙂
When you pass non-POD arguments into functions it is usually more efficient to pass them as references, because it means you don’t create a copy of them. (POD => Plain Old Data: int, char, etc).
When you pass an argument by reference it can introduce confusion, is the argument being passed by reference, or is it going to be assigned a value within the function and used as an out variable? So it helps readers understand the code if you make you references const.
Of course adding comments to your code makes a big difference. You know what it is doing and WHY now, but give it 6 months time and you will think an alien wrote it.
You should always have a think about using literal values, i.e. name and age. OK in this case its not too much of an issue, but what about wrapping them up in a map with the replacement values? It would make your code shorter and more extendable?
std::string Replace(std::string text,
Why are you passing text by value ? Best practice now is to pass via string_view instead.
std::function<std::string(std::smatch)> func)
Passing a std::function is less efficient than making it a template like all the algorithms and various places in the standard library. If this is just a toy example for the purpose of getting familiar with std::function, then fine. But if you are looking for the best way to customize an algorithm with caller-supplied code, use a template and pass it as a type rather than a pointer (your example call of using a lambda is good).
std::string result = "";
As with your previous line, you don’t need to initialize string to produce an empty string. That’s what the default constructor does. If you do need to name an empty string, use { } not "". The latter invokes the general const char* constructor which goes to a lot of work to figure out it’s actually empty!
OK, I see you are re-writing text for each match in the string, and that’s why you passed it by value. But passing a string by value is a red flag for code reviews so it will bother readers even if you did have a reason for it.
It is inefficient to keep rewriting text with the tail end of the string, each time through the loop. If you used a string_view you could efficiently trim the prefix instead.
I’m not familiar with the regex class, but I know that the PCRE it was modeled on can find all matches in a string without having to do that. Doesn’t this class let you find all matches via an iterator or something?