I'll start by reviewing the code as it stands, then add some commentary about how I think I'd do this job instead.
I'd say your code is quite readable--while I personally prefer snake_case variable names over the camelCase you've used, it's at least a decent convention and you've followed it fairly consistently.
The names themselves aren't quite as meaningful as I'd prefer though. The prime example would be using vector
as the name of the variable holding the input. I see two problems here. First, you've re-used a name from the standard library for an entirely different purpose (a variable instead of a template). While I'm somewhat more tolerant of re-using names for different purposes than many, it seems ill-advised in this case. Second, and more importantly, vector
only tells us about an incidental detail of how the input data happens to be stored. It would be much more useful to give it a name that reflects its purpose and use instead--input
for one possible example.
That brings us to another point: I think your code specifies types to an unnecessary degree--to the point that its flexibility is substantially reduced. In some cases you can justify reduced flexibility when the greater specialization makes the code much easier to read and/or use--but at least in this case, we don't seem to be gaining a lot.
Finally, I think the code combines two rather separate tasks:
- Breaking a collection of inputs into contiguous ranges, and
- Converting contiguous ranges to readable strings.
It seems to me that the code would benefit from keeping these two operations separate.
For the first of these, I think @iavr had (at least roughly) the right idea: it should be implemented as a generic algorithm that takes input as a pair of iterators. Unlike his solution, I'd also write the output via an iterator rather than directly to a stream (-like object) using an insertion operator.
This makes it trivial to write the output to a string (via a stringstream), or directly to any other sort of stream, or to a collection (e.g., vector or deque) of the right type.
To do this, I'd start by defining a type to hold a range, and handle insertion of that type into a stream:
template <class T>
class range_t {
T begin, end;
public:
range_t(T begin, T end) : begin(begin), end(end) {}
friend std::ostream &operator<<(std::ostream &os, range_t const &r) {
os << r.begin;
if (r.end != r.begin)
os << "-" << r.end;
return os;
}
};
The stream insertion operator also handles conversion of that object to a string (the second task noted above) by writing to a stringstream. Depending on the situation, it can be beneficial to add a to_string
function to carry out that conversion directly. This is largely an optimization though, adding more code to avoid the overhead of a stringstream. I'll leave it out for now, but note that it's generally fairly trivial to add as long as T is a type that itself has a to_string
(which it does for the normal integer types in C++11).
Then for the sake of convenience, I'd define a function template to create an object of that type (this allows the type parameter to be deduced from the passed type rather than having to be specified explicitly):
template <class T>
range_t<T> range(T begin, T end) { return range_t<T>(begin, end); }
With those in place we can get to the real code to create a series of ranges from some input data:
template <typename RIt, typename OutIt>
void contiguous_ranges(RIt begin, RIt end, OutIt result) {
std::sort(begin, end);
auto pos = begin;
for (++pos ; pos != end; ++pos) {
if (*pos > *(pos - 1) + 1) {
*result++ = range(*begin, *(pos - 1));
begin = pos;
}
}
*result++ = range(*begin, *(end-1));
}
One minor note: although this uses random-access iterators for the input (thus "RIt" for the type name), the random access is needed only for the std::sort
. The rest of the algorithm only requires input iterators (and it's fairly easy to use a different sorting algorithm that doesn't require random access iterators, though typically with some loss of efficiency).
You'd put these together something like this:
std::vector<int> input {1,2,3,4,5,7,7,8,8,8,10,15,10,11,12,88,87,86};
contiguous_ranges(input.begin(), input.end(),
infix_ostream_iterator<range_t<int> >(std::cout, ", "));
[To avoid an extra comma at the end of the output, I've used the infix_ostream_iterator
from a previous question though you could substitute a normal std::ostream_iterator
if you didn't mind an extra comma after the end of the output.]
At least to me, this seems to combine simplicity in use, readability (especially to anybody already accustomed to standard algorithms) and flexibility--it's able to produce output to a string, a stream, or essentially any collection that's accessible via an output iterator.