Unexpected Output from solution of Begginer's C++ Exercise

Question

I needed to create some kind of a bus route handbook (just practicing around), so I wrote this.

#include <iostream>
#include <string>
#include <vector>
#include <map>

using namespace std;

vector<string> GenWay(int length){
    vector<string> res;
    string stop;
    for(int i = 0; i<length; i++){
        cin >> stop;
        res.push_back(stop);
    }
    return res;
}

int isVinMap(vector<string> vs, map<int,vector<string>> miv){
    for(const auto& [key, vvalue]:miv){
        if(vvalue == vs) return key;
    }
    return 0;
}


int main(){
    int q=0;
    cin >> q;

    map<int,vector<string>> ways;
    string stop="";
    int command=0;
    int next_way = 1; //last+1 //last = 0

    for (int i = 0; i < q; i++){
        cin >> command;
        vector<string> new_way = GenWay(command);
        int check = isVinMap(new_way,ways);

        cout << check << !static_cast<bool>(check); //Debug code

        if(!static_cast<bool>(check)){
            cout << "New bus " << next_way << endl;
            ways[next_way] = GenWay(command); //next_way is not controlled by user
            next_way++;
        } else{
            cout << "Already exists for " << check << endl;
        }
    }
}

Here what it should do:

First input must be the quantity of coming commands. (q) (e.g. 4) (no output)

There's only one command that can be processed after first, and it has next format: number_of_bus_stops stop1 stop2 stop3... (e.g. 2 m k). It adds an entry to map<int,vector<string>> ways, about the marchrute (The number of marchrute is not defined by user, and is next_way, which should increase after each new entry). If the same marchrute appear in other entries, it tells that it already exist and print the number of marchrute which contains this way (isVinMap method checks that and gives the number of marchrute)(ways with different order of stops are different), and tells that new bus created by saying (e.g. New bus 1) if adding of an entry is succesful.

However it doesn't work as it supposed to. Output is nearly unpredictable. I'm working in EclipseIDE on Winx64 system and got this output:

>4
>2 m k
01New bus 1
>3 k r s
01New bus 2
20Already exists for 2
20Already exists for 2

Seems like cycle goes on for two more times, after first two commands, but it doesn't invite me to input anything.

And, of course, any criticism of the code is appreciated! and sorry for lack of documentation and commentaries in the code, it wasn't supposed that i would have needed to work for that long on this one.

Answer 1

Shortening the code to just the relevant parts:

for (int i = 0; i < q; i++)
{
    cin >> command;
    vector<string> new_way = GenWay(command);
    if(!isVinMap(new_way, ways))
    {
        ways[next_way] = GenWay(command); // <-- (!)
// ...

At the marked line, you are producing an entirely new vector, which might be, apart from size, totally unrelated to the one created previously (stored in new_way), and possibly worse, might already be contained in the map (there's no new check for!).

Most likely, you intended to insert the way already read previously instead:

        ways[next_way] = new_way;

I tried it and got some problems with debugger... I try step by step execution, but it just skips the cin input. [...]

Well, then you could create some dummy data instead, e. g. by using a random number generator:

char const* stopNames[] = {"a", "b", "c, /*... */ };
// (you'll need to find out how large it should be to produce sufficient different

std::mt19937 gen(std::random_device()());
std::uniform_int_distribution<> lengths(10, 12);
// (routes with length in between 10 and 12 - or whatever else appears appropriate
//  to you)

std::uniform_int_distribution<> stops(0, sizeof(stopNames)/sizeof(*stopNames) - 1);

//cin >> command;
command = lengths(gen);

//cin >> stop
stop = stopNames[stops(gen)];

In other scenarios, you might want to have entirely static data, e. g. just having a std::vector<std::vector<std::pair<std::string, std::string>>> where you have all data statically stored.

Huh, std::pair<std::string, std::string>??? Well, that way you could serve the second (bad) read of ways that has been eliminated by this answer; this shows that test data has to be selected carefully sometimes... If in doubt, you might prefer range-checking (safe) accessors (std::vector::at) over non-checking ones (operator[]) for retrieving test data.

Reducing complexity of test-data sometimes is appropriate, too, e. g. in given case, you could have used a constant length for all of your routes, possibly resulting in test-data like

std::array<std::array<std::string, 12>, 10> routes({ ... });

producing 10 routes all with length 12...