Core Dump when trying to make values in vector unique

Question

I'm doing an c++ assignment and I'm a complete newbie to c++. The idea of the assignment is to create an 11 players team form 22 available players. You've to choose 1 from 3 available goalkeepers, 4 from 7 defense players, 4 from 8 middle field players and 2 from 4 attackers (this is a soccer team). Now the possible combinations for all of that are 44100. It's a requirement that I used enumerations for the mentioned team parts and I've to give a unique output each time. I'd tried many approaches, but ended up using vectors to do the job. The logic seems to be write to me and the debugging doesn't really help as I get segmentation fault. It would be great if someone could give a hand. I hope the description is clear enough. Here is the code:

#include <string>
#include <vector>
#include <ctime>
#include <iostream>

  enum goal {
        Neuer, terStegen, Trapp
    };

    enum def {
        Boateng, Ginter, Hector, Hummels, Ruediger, Schulz, Suele
    };

    enum mid {
        Brandt, Can, Draxler, Goretzka, Guendogan, Kroos, Mueller, Rudy
    };

    enum att {
        Gnabry, Reus, Sane, Werner
    };

    std::string chooseGoal(int x) {
        switch (x) {
        case 0: return "Neuer";
        case 1: return "ter Stegen";
        case 2: return "Trapp";
        }
    }

    std::string chooseDef(int x) {
        switch (x) {
        case 0: return "Boateng";
        case 1: return "Ginter";
        case 2: return "Hector";
        case 3: return "Hummels";
        case 4: return "Ruediger";
        case 5: return "Schulz";
        case 6: return "Suele";
        }
    }

    std::string chooseMid(int x) {
        switch (x) {
        case 0:return"Brandt";
        case 1:return "Can";
        case 2:return "Draxler";
        case 3:return "Goretzka";
        case 4:return "Guendogan";
        case 5:return "Kroos";
        case 6:return "Mueller";
        case 7:return "Rudy";
        }
    }

    std::string chooseAtt(int x) {
        switch (x) {
        case 0: return "Gnabry";
        case 1: return"Reus";
        case 2: return "Sane";
        case 3: return "Werner";
        }
    }

    std::vector<std::string> createVectorDef() {
        std::vector<std::string> v;
        std::srand(std::time(NULL));
        for (int i = 0; i < 4; i++) {
            int x = rand()%7;
            v.push_back(chooseDef(x));
        }
        return v;
    }

    std::vector<std::string> createVectorMid() {
        std::vector<std::string> v;
        std::srand(std::time(NULL));
        for (int i = 0; i < 4; i++) {
            int x = rand() % 8;
            v.push_back(chooseMid(x));
        }
        return v;
    }

    std::vector<std::string> createVectorAtt() {
        std::vector<std::string> v;
        std::srand(std::time(NULL));
        for (int i = 0; i < 2; i++) {
            int x = rand() % 4;
            v.push_back(chooseAtt(x));
        }
        return v;
    }

    std::vector<std::string> makeUniqueDef(std::vector<std::string> v) {
        for (int i = 0; i < v.size()-1; i++) {
            for (int j = i + 1; j < v.size(); j++) {
                if (v[i] == v[j]) {
                   // v.clear();
                    v = createVectorDef();
                    makeUniqueDef(v);
                } else {
                    continue;
                }
            }
        }
        return v;
    }

    std::vector<std::string> makeUniqueMid(std::vector<std::string> v) {
        for (int i = 0; i < v.size()-1; i++) {
            for (int j = i + 1; j < v.size(); j++) {
                if (v[i] == v[j]) {
                    v = createVectorMid();
                    makeUniqueMid(v);
                } else {
                    continue;
                }
            }
        }
        return v;
    }

    std::vector<std::string> makeUniqueAtt(std::vector<std::string> v) {
        for (int i = 0; i < v.size()-1; i++) {
            for (int j = i + 1; j < v.size(); j++) {
                if (v[i] == v[j]) {
                    v = createVectorAtt();
                    makeUniqueAtt(v);
                } else {
                    continue;
                }
            }
        }
        return v;
    }

    void displayVector(std::vector<std::string> v) {
        for (int i = 0; i < v.size(); i++) {
            if (i == v.size() - 1) {
                std::cout << v[i] << '.' << std::endl;
                break;
            }
            std::cout << ' ' << v[i] << ',';
        }

    }

    int main() {
        // Random Gen
        std::srand(std::time(NULL));
        std::cout << "Tor: " << chooseGoal(rand() % 3) << std::endl;
        //Vector for defence and display
        std::vector<std::string> def = createVectorDef();
        def = makeUniqueDef(def);
        std::cout << "Abwehr:";
        displayVector(def);
        // Vector for mid and display
        std::vector<std::string> mid = createVectorMid();
        mid = makeUniqueMid(mid);
        displayVector(mid);
        std::cout << "Mittelfeld:";
        displayVector(mid);
        // Vector for atta and display;
        std::vector<std::string> att = createVectorAtt();
        att = makeUniqueAtt(att);
        //    std::cout << "Angriff:";
        displayVector(att);



    }

*(this is a soccer team)* Thank Crom. That would be one freaking weird Hockey team. — user4581301, Nov 14 '18 at 00:06
Debugger may not be necessary. Turn up the the compiler warnings or start heading them. You have many functions that promise to return a value, but do not. Add a `default: throw std::runtime_error("Invalid input in ");` to the switch statements in the `choose` functions to see if any of them are exiting without selecting a player. — user4581301, Nov 14 '18 at 00:12
Another suggestion: Store the players in a `vector` by position. When a player is chosen, remove them from the `vector` so they cannot be chosen again. This turns all of the `choose` functions into one function that receives a reference to a different `vector` for each position. — user4581301, Nov 14 '18 at 00:14
@user4581301 I laughed at the first comment so hard :D. I can't find any function that doesn't return, could you point that out? regarding the idea with storing by position, you mean to insert the players int value from the enum in a vector? because I'm forced to use enum by the professor — noobnomore, Nov 14 '18 at 00:20
@user4581301 chooseGoal isn't supposed to return any thing, it'll just print out, it doesn't need to have a vector or makeUnique of its own, since it's so simple as there's only one option to output each run and it'll always be unique — noobnomore, Nov 14 '18 at 00:25
It returns a `string` If you print out that `string` and no string was returned you get Undefined Behaviour. — user4581301, Nov 14 '18 at 00:29
You can put whatever you want in the `vector`. Take the defender `vector`, `std::shuffle` it and pick the first N players from it. That's the defenders for team 1. Pick the next N players and that's the defenders for team 2. repeat for other positions. Whole program is reduced to a couple functions and some `vector` initialization. — user4581301, Nov 14 '18 at 00:31
I think I just spotted your bug. You have poorly controlled recursion in the `makeUnique` functions. If you keep getting unlucky and don't get a unique player the program runs off the end of the stack. Don't use recursion with the exit condition based on random numbers. You never know when you'll get out. — user4581301, Nov 14 '18 at 00:37
Another bug that exacerbates the main problem: You reseed the random number generator on the `createVector` functions. This will most likely happen many times a second and result in the same seed and same random sequence over and over, pretty much guaranteeing that the player selections never stop colliding. You almost never want to call `srand` more than once a program. Fixing this makes your program work more often than not. — user4581301, Nov 14 '18 at 00:45
Question: *4 from 7 defense players*. How? For two teams you will need 8 defensive players. Another question. Where the heck is Klose? He's only 40. Couple more years in him. — user4581301, Nov 14 '18 at 01:03
@noobnomore *chooseGoal isn't supposed to return anything* -- `std::string chooseGoal` -- When you state that a function returns something, you cannot break that contract. You must return a value. — PaulMcKenzie, Nov 14 '18 at 01:19
@user4581301 I can't really do anything about the numbers of players, it's given like this in the assignment and only one team is needed, not two. I'll ask my profiessor about Klose :D — noobnomore, Nov 14 '18 at 09:43
@PaulMcKenzie yes, but I don't think this is the problem here — noobnomore, Nov 14 '18 at 09:44
@noobnomore you are correct. The problem in the code causing the crash you're dealing with is uncontrolled recursion. That said, technically a compiler is allowed to create wildly bad code as a result of undefined behaviour, but this wildly bad code should only be reached if the path that does not return is taken. Throwing an exception if the path is taken is among the right solutions. I believe it to be the rightest because it detects and forces you to deal with a programming error should it occur. — user4581301, Nov 14 '18 at 19:05

score 0 · Accepted Answer · edited Jun 20 '20 at 09:12

Problem 1

Recursion with an exit case that depends on probability. This always runs the risk of getting unlucky and not exiting before the recursion consumes all Automatic storage.

Problem 2

Repeated calling of srand. Why this is bad is covered here: srand() — why call it only once?

Solution

Don't recurse.

Place players into vectors and use standard library tools to shuffle the vectors and ensure no one can ever be selected twice by removing them from the vector after they are selected.

Here is an example demonstrating only the defenders with comments added where I felt explanation was required.

#include <string>
#include <vector>
#include <ctime>
#include <iostream>
#include <algorithm>
#include <stdexcept>
enum def {
    Boateng, Ginter, Hector, Hummels, Ruediger, Schulz, Suele
};

std::string chooseDef(def x) // use the type, not int. Compiler can help trap errors
{
    // There are better ways to do this. If you put all of the players
    // into one enum you can have one mapping function
    switch (x) {
        // using enumerated value rather than magic numbers
        case Boateng:  return "Boateng";
        case Ginter:   return "Ginter";
        case Hector:   return "Hector";
        case Hummels:  return "Hummels";
        case Ruediger: return "Ruediger";
        case Schulz:   return "Schulz";
        case Suele:    return "Suele";
        default: throw std::runtime_error("Invalid input in chooseDef");
        // added to trap error if called with bad value
    }
}

std::vector<def> makeUniqueDef(std::vector<def> & defenders) {
    // add first four players in vector to vector to be returned. Probably should have a 
    // test here to make sure there are four player in the vector.
    std::vector<def> players(defenders.begin(), defenders.begin() +4);
    
    // remove players from source vector so they can't be reselected 
    defenders.erase(defenders.begin(), defenders.begin()+4);
    return players;
}

void displayVector(std::vector<def> v) {
    for (size_t i = 0; i < v.size(); i++) {
        // note only converting from enum to string on display
        // this is likely closer to what your instructor wants.
        std::cout << chooseDef(v[i]) << '.' << std::endl;
    }
}

int main() {
    // Random Gen call once and only once.
    // if you need to call it more than once, rand is probably not the tool you need
    std::srand(std::time(NULL));

    // put all defenders in vector
    std::vector<def> defenders = {Boateng, Ginter, Hector, Hummels, Ruediger, Schulz, Suele};

    // mix them up to make for fair selection
    std::random_shuffle (defenders.begin(), defenders.end());
    // random_shuffle is obsolete, but used because it's closer to rand/srand
    // prefer modern C++'s std::shuffle and random library

    // select defenders
    std::vector<def> team1def = makeUniqueDef(defenders);


    // print defenders
    displayVector(team1def);
}

Sample output:

Team 1
Hummels.
Boateng.
Ginter.
Hector.
Team 2
Suele.
Ruediger.
Schulz.
Kimmich.

according to the assignment I only need to output one team only. The second problem is that I need to have a unique team constellation everytime I run the code and I've to have 4 enums for each part of the team (def, att,mid,goalkeeper). Your code will work, but will not output a unique constellation everytime, as the players space to choose from is so small. — noobnomore, Nov 14 '18 at 11:53
@noobnomore I misread the significance to the 22 players. My apology, and my apologies to Mr. Kimmich. Your services are no longer required. — user4581301, Nov 14 '18 at 18:58
You cannot have a unique team every time for the reason you described: The pool of players is too small. No matter how you organize or arrange the players, you are doomed to repetition and you will be hard pressed to beat this solution, though it can easily be improved by using a better [random number generator](https://channel9.msdn.com/Events/GoingNative/2013/rand-Considered-Harmful). If you do not like the solution at least take the time to understand and resolve the problems outlined at the top. — user4581301, Nov 14 '18 at 18:58
you sir, are my savior! I found what the error was, it was actually the repeated usage of srand(time(NULL)) and as soon as I removed it, the code worked like magic (I didn't try the code on windows where I ogriginally wrote it, but on macos xcode). I know that you can't completely eliminate the repetition, but may be lessening them. This was a very interesting assignment and I think I'l try to do it in a couple of different ways for learning purposes. Thank you so much for the time and effort and all the references and tip, but above all your sense of humor :D — noobnomore, Nov 14 '18 at 20:37

Core Dump when trying to make values in vector unique

1 Answers1

Problem 1

Problem 2

Solution