That's an interesting case. First, I thought that the function should be a function template for a pair of input iterators and an output iterator, as that's an idiomatic approach taken by many STL algorithms and what the function matches this pattern. But given that \n
plays a crucial role here, passing iterators to an underlying std::ifstream
doesn't make much sense (it consumes words, not lines).
Nevertheless, let me share my thoughts besides.
Why does it have to be a static data member s_bInComment
? I assume removeComment
is static, too? This sounds like it's somewhat decoupled from the rest of the Parser
class, which might have some instance-specific state. Maybe it makes things cleaner and more testable if you create a small class only for the removal of comments, which improves encapsulation and circumvents the static
variable - non-const
, static
variables are too global for my taste, and they are generally thread-unsafe.
The function is very long, and it contains many nested branches. I think it's common sense that this is not very readable and should be split into smaller functions. If you give them meaningful names, you can even remove some of your comments (e.g. hasPartialEndToken(str)
or startsWithGlobalToken(str)
).
If you care about allocations and C++17 is available, you could change the way this functions passes its result back to the caller, i.e., via a std::vector<std::string_view>
. The lifetime of the input argument is controlled by the caller, so it is probably sufficient to not copy the non-comment parts of the string into a result, but instead create a collection of views on the input. As std::string::substr
and concatenating strings with operator +
are possibly expensive operations, you might keep this option in mind (and of course, only go with it after you made sure that performance/memory footprint is important, you profiled your code and so on...)
Last, a more general question - why aren't you using <regex>
? The API is not famous for its ease of use, but you do a lot of manual work that could be outsourced to the standard library, and developers maintaining such a piece of code wouldn't be surprised to find some regular expressions in it. Here's a small class has the same functionality as the snippet you posted:
class CommentRemover {
public:
std::string strip(const std::string& input);
private:
bool isInComment = false;
static const std::regex restOfLine;
static const std::regex partialInline;
static const std::regex multiLineBegin;
static const std::regex multiLineEnd;
};
with the actual std::regex
objects being defined as
const std::regex CommentRemover::restOfLine{"(//|#)"};
const std::regex CommentRemover::partialInline{"(/\\*.*?\\*/)"};
const std::regex CommentRemover::multiLineBegin{"(/\\*.*(?!\\*/))"};
const std::regex CommentRemover::multiLineEnd{"(/\\*.*(?!\\*/))"};
and the implementation of the member function looks like
std::string CommentRemover::strip(const std::string& input)
{
std::smatch match;
// Save some typing below:
const auto search = [&match, &input](const std::regex& re){
return std::regex_search(input, match, re); };
if (isInComment && search(multiLineEnd)) {
isInComment = false;
return strip(match.suffix().str());
} else if (search(partialInline))
return match.prefix().str() + strip(match.suffix().str());
else if (search(restOfLine))
return match.prefix().str();
else if (search(multiLineBegin)) {
isInComment = true;
return match.prefix().str();
}
return input;
}
This is not a prime example for readability, and the regex specifications are admittedly cryptic. But I think it reduces the likelihood of bugs due to less code and no manual looping/indexing. Plus, it allows for concentrating the complexity of the operation into the regex definitions and the if
-else if
ordering.