I will try to tell you what is wrong with your algorithm and why.
You do loop twice over the input string which is most certainly wrong
Your first loop does provide bGroups
and wGroups
as a result. We immediatly notice, that wGroups
is not used at all and can be removed completely. bGroups
is used only as limit in the second loop where you loop over all characters again and have to find all groups anyway. So why bother with finding them firsthand? If the second loop iterates over all characters (like the first one does) it can do the group search on its own and we can delete the first loop completely.
However your second loop contains nested while loops that do not stop at the last character. It needs to know the number of groups in advance. But it does not respect the length of the string anyway as it accesses s[s.size()]
if the last character is a 'B'
. This is guaranteed to be '\0'
in C++14 (and thus different than 'B'
) but may work on older compilers just by accident. The bottom line is that your algorithm would be broken for a different container or of you were searching '\0'
characters.
As a learning you should avoid parsing where states are represented by a position in a sequential code. Hold state in variables and have a single flat loop that can safely break.
You also can see your nested loop counting is wrong as there is code duplication. There is an if
statement to have the very same code in a different order depending on the start character.
Try to format your code pretty
#include<iostream>
should read #include <iostream>
Use std::string.size()
This is the standard how to determine the length of a container and iterate over it. It is a pretty bad idea to hold the length in a seperate variable that must match the actual length. The traditional container loop looks like
for (int i = 0; i < s.size(); i++) {
char c = s[i];
// ...
}
The modern range based loop - stick to that - looks like
for (const auto & c : s) {
// ...
}
Do not construct a vector like an array
Instead of doing so and filling with the operator[]()
you should use std::vector.push_back()
to grow in size when you need.
Separate I/O from algorithm
Make a testable function without I/O that you call from main. The code should be structured somewhat like
std::vector<int> puzzle(std::string s)
{
std::vector<int> grpSize;
// ...
return grpSize;
}
std::string input() {
// ...
return s;
}
void output(std::vector<int> v) {
// ...
}
int main() {
std::string s(input());
output(puzzle(s));
}
using namespace std;
This line is found in many tutorials and even in IDE templates. You should use this very carefully in a limited scope only. Never use this in header files or before include lines as it may introduce name conflicts. When you fully understand this you may decide to use it in *.cpp files e. g. in an output function.
finally your algorithm should look somewhat like
std::vector<int> puzzle(std::string s)
{
std::vector<int> grpSize;
// as we are interested in 'B' groups only
// we start in state 'W' to catch the first 'B'
char state{'W'};
for (const auto & c : s) {
if (c == 'B') {
if (state != 'B') {
grpSize.push_back(0);
}
++grpSize.back();
}
state = c;
}
return grpSize;
}