4
\$\begingroup\$

I am contributing a solution to the following task on Rosetta Code:

Task
Split a string into comma (plus a blank) delimited strings based on a change of character (left to right).

Show the output here (use the 1st example below).

Blanks should be treated as any other character (except they are problematic to display clearly). The same applies to commas.

For instance, the string:

gHHH5YY++///\

should be split and show:

g, HHH, 5, YY, ++, ///, \

Here is the code:

// Solution for http://rosettacode.org/wiki/Split_a_character_string_based_on_change_of_character
#include<string>
#include<iostream>
auto spliter(const std::string &input) {
   auto firstCommaPast = false;
   std::string res ="";
   auto prev = '\0';
   for(auto it = input.cbegin(); it != input.cend();++it) {
      if(*it!=prev) {
         if(!firstCommaPast) {
            firstCommaPast = true;
         } else {
            res+=", ";
         }
      }
      res+=*it;
      prev=*it;
   }
   return res;
}

int main() {
   auto input = R"(gHHH5  ))YY++,,,///\)";
   std::cout<<spliter(input);
}  

I am fairly new to C++11/14 so any suggestions here will really help.

\$\endgroup\$
  • \$\begingroup\$ I would suggest the standard algorithm adjacent_find. \$\endgroup\$ – cqdjyy01234 Feb 25 '17 at 15:58
2
\$\begingroup\$

Algorithm

At least at first glance, your algorithm appears to be unnecessarily complex. I'm not entirely sure what firstCommaPast is intended to accomplish. It seems to be trying to give some sort of special treatment to commas, even though the directions explicitly state that commas should be treated exactly like any other character.

Whitespace

I prefer to see a blank line between the #includes and other code. I also prefer to see a space between the include and the < or " that follows it:

#include <string>
#include <iostream>

auto spliter(const std::string &input) {

These don't make any difference to functionality, but fit pretty well with widely accepted practice. I won't try to comment on whether the opening curly brace ({) belongs on the same line as the function header, or on the next line by itself. That's widely debated, and I don't think there's any real consensus on which is preferred.

Initialization

Given that it's going to be an empty string either way, I'd rather use std::string res; rather than std::string res="";. The latter will typically add a little extra work without accomplishing anything useful.

Iterators vs. indexing

The primary reason for using iterators is to support generic algorithms. Using them for a loop outside of a generic algorithm is somewhat questionable. In this case, I see no real improvement over just using an integer type and indexing into the string.

Logic Error

Right now, your code assumes that the first character of a string can't be a '\0'. While that's probably at least somewhat unusual, unlike with C strings, it is actually allowed/supported in a std::string (and if it's present, your code won't really work as intended).

Personally, I think it's easier to initialize prev to the first character of the input string, then have the loop proceed from input[1] through the end of the string. This way we don't have to initialize prev to a value that can't be in the string (which is good, because there is no such character).

Function Naming

A function does something. As such, its name should normally be a verb, not a noun. Although it may be open to argument whether the best name for what it does is splitting, let's just go with that general idea--in which case its name should be split, not splitter.

Resulting Code

Incorporating the elements above, the code could end up something like this:

#include <string>
#include <iostream>

auto split(const std::string &input) {
    auto prev = input[0];
    std::string res{ prev };

    for (size_t i = 1; i < input.length(); i++) {
        if (input[i] != prev)
            res += ", ";
        prev = input[i];
        res += prev;
    }
    return res;
}

int main() {
    auto input = R"(gHHH5  ))YY++,,,///\)";
    std::cout << split(input);
}
\$\endgroup\$
  • \$\begingroup\$ Thanks Jerry -- that's really useful, very much appreciated. You are correct about iterators v. indexing on strings, I guess I use it when I'm doing generic types and here it's really only a string, so doesn't add value and makes things slightly more complex. \$\endgroup\$ – Jubin Chheda Feb 25 '17 at 21:03
-1
\$\begingroup\$

Looks perfect. You have the input string hardcoded here, is there a possibility of having a new line character in your input (if input is given in run-time).??

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! May be you could expand on perfect? You may refer to How do I write a good answer? if you need some further information. \$\endgroup\$ – Incomputable Feb 25 '17 at 13:33
  • \$\begingroup\$ I was wondering if there is a better way to iterate and build a string result in a scenario like this one. \$\endgroup\$ – Jubin Chheda Feb 25 '17 at 14:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.