Skip to main content
be more specific on size()
Source Link
stefan
  • 3k
  • 8
  • 17

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. Instead of

for (int i = 0; i < bGroups; i++)
{
    cout << grpSize[i] << " ";
}

The traditional container loop looks like

for (int i = 0; i < sgrpSize.size(); i++)  
{
    char c = s[i];
cout << grpSize[i] << //" ...";
}
for (const auto & cn : sgrpSize) {
    //cout ...<< n << " ";
}

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];
    // ...
}
for (const auto & c : s) {
    // ...
}

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. Instead of

for (int i = 0; i < bGroups; i++)
{
    cout << grpSize[i] << " ";
}

The traditional container loop looks like

for (int i = 0; i < grpSize.size(); i++) 
{
    cout << grpSize[i] << " ";
}
for (const auto & n : grpSize) {
    cout << n << " ";
}
Source Link
stefan
  • 3k
  • 8
  • 17

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;
}