Thread: Segmentation Fault issue

Hi,

For my final project this semester, we were asked to make a modified poker-like game in which the player must try his/her best to match a type of poker hand and they are paid in accordance to what hand they create and how much they wager. Anyway, I believe I have most of the assignment finished (I still have to setup a wager system, a better card printing function, and a few other things). Anyway, The code I have this far is giving me a segmentation fault after completing one cycle of the while loop in the main function. I tried debugging my code with valgrind and it is directing me to my call to "cin." I cannot figure out why cin would cause a segfault. Anyway, here's my code. Different code blocks represent different source and header files

Homework5.cpp

Code:

//============================================================================
// Name        : Homework5.cpp
// Author      : 
// Version     :
// Copyright   : Your copyright notice
// Description : Hello World in C++, Ansi-style
//============================================================================

#include <iostream>
#include "hand.h"
#include "deck.h"
#include "prototypes.h"
using namespace std;

int main()
{
    hand myHand;
    deck myDeck;
    int i,j, handType;
    string answer;
    bool playAgain = true;

    //play game
    while(playAgain == true)
    {
        myDeck.shuffle();
        //print out deck for debugging purposes only
        for(i=0;i<52;++i) cout<<myDeck.getValue(i)<<"*"<<myDeck.getSuit(i)<<endl;

        j = deal(myDeck,myHand);
        cout<<myHand.getValue(0)<<"*"<<myHand.getSuit(0)<<"  "<<myHand.getValue(1)<<"*"<<myHand.getSuit(1)<<"  "<<myHand.getValue(2)<<"*"<<myHand.getSuit(2)<<"  "<<myHand.getValue(3)<<"*"<<myHand.getSuit(3)<<"  "<<myHand.getValue(4)<<"*"<<myHand.getSuit(4)<<endl;

        for(i=0;i<5;++i)
        {
            cout<<"Discard card number "<<i+1<<"?";
            cin>>answer;
            cout<<endl;

            if(answer == "yes")
            {
                myHand.setValue(i,myDeck.getValue(j));
                myHand.setSuit(i,myDeck.getSuit(j));
                ++j;
            }
            cout<<myHand.getValue(0)<<"*"<<myHand.getSuit(0)<<"  "<<myHand.getValue(1)<<"*"<<myHand.getSuit(1)<<"  "<<myHand.getValue(2)<<"*"<<myHand.getSuit(2)<<"  "<<myHand.getValue(3)<<"*"<<myHand.getSuit(3)<<"  "<<myHand.getValue(4)<<"*"<<myHand.getSuit(4)<<endl;
        }

        handType = myHand.handType();

        cout<<"Hand Type: "<<handType<<(handType > 0 ? '\a':' ')<<endl;
        cout<<endl<<"Play Again? ";
        cin>>answer;
        if(answer != "yes") playAgain = false;
    }
    return 0;
}

functions.cpp

Code:

#include "hand.h"
#include "deck.h"

int deal(deck &myDeck, hand &myHand)
{
    int i;
    for(i=0; i<5;++i)
    {
        myHand.setValue(i,myDeck.getValue(i));
        myHand.setSuit(i,myDeck.getSuit(i));
    }
    return i;
}

card.h

Code:

#ifndef CARD_H_
#define CARD_H_

class card
{
    private:
        int value;
        int suit; // 1=clubs, 2=spades,3=hearts,4=diamonds

    public:

        card(){setValue(2); setSuit(1);}
        card(int v, int c) {setValue(v); setSuit(c);}

        void setValue(int v) {value = v;}
        void setSuit(int c) {suit = c;}
        int getValue(){return value;}
        int getSuit(){return suit;}
};



#endif

deck.h

Code:

#ifndef DECK_H_
#define DECK_H_

#include "card.h"

class deck
{
    private:
    card cards[52];

    public:

    deck();

    void setValue(int i, int v) {cards[i].setValue(v);}
    void setSuit(int i, int c) {cards[i].setSuit(c);}
    int getValue(int i) {return cards[i].getValue();}
    int getSuit(int i) {return cards[i].getSuit();}

    void shuffle(void);

};
#endif

deck.cpp

Code:

#include <ctime>
#include <cstdlib>
#include <unistd.h>
#include<iostream>
#include "deck.h"
using namespace std;

deck::deck()
{
    int i=0,j,k;

    for(j = 1; j < 5; ++j)
    {
        for(k = 2; k<15 ;++k)
        {
            cards[i].setValue(k);
            cards[i].setSuit(j);
            ++i;
        }
    }
}

void deck::shuffle()
{
    card temp;
    int i,j;
    cout<<"shuffling..."<<endl;
    for(i = 0; i < 52; ++i)
    {
        temp = cards[i];
        sleep(1);
        srand(time(NULL));
        j = rand()%51;
        cards[i] = cards[j];
        cards[j] = temp;
    }
}

hand.h

Code:

#ifndef HAND_H_
#define HAND_H_

#include "card.h"
#include <iostream>
class hand
{
    private:
        card cards[5];

        int valueCounter[15]; // J=11, Q=12, K=13, A=14
        int suitCounter[5];  // 1=clubs, 2=spades,3=hearts,4=diamonds
    public:
        hand() {}

        void initializeCounters() {int i=0; while(i<15) {valueCounter[i] = 0, suitCounter[i] = 0; ++i;}}
        void countCards(void);

        void setValue(int i, int v) {cards[i].setValue(v);}
        void setSuit(int i, int c) {cards[i].setSuit(c);}
        int getValue(int i) {return cards[i].getValue();}
        int getSuit(int i) {return cards[i].getSuit();}

        int handType(void);
        //string printHand(void);
};
#endif

hand.cpp

Code:

#include "card.h"
#include "hand.h"
#include <iostream>
using namespace std;
void hand::countCards(void)
{
    initializeCounters();
    int cardVal = 2, suitVal=1, cardNum=0;

    while(cardNum<5)
    {
        while(cardVal<15)
        {
            if(cardVal == cards[cardNum].getValue()) {++valueCounter[cardVal]; cardVal=2; break;}
            ++cardVal;
        }
        while(suitVal<5)
        {
            if(suitVal == cards[cardNum].getSuit()) {++suitCounter[suitVal]; suitVal=1; break;}
            ++suitVal;
        }
        ++cardNum;
    }
}

int hand::handType()
{
    enum hands {none,onePair, twoPair, threeOfAKind, straight, flush, fullHouse, fourOfAKind, straightFlush, royalFlush};

    countCards();
    int i,j, pair = 0, threeKind = 0;
    for(i=1; i<5; ++i)
    {
        if(suitCounter[i] == 5)
        {
            if(valueCounter[10] == 1 && valueCounter[11] == 1 && valueCounter[12] == 1 && valueCounter[13] == 1 && valueCounter[14] == 1) return (int)royalFlush;
            else
            {
                for(j=2;j<15;++j)
                {
                    if(valueCounter[j] == 1 && valueCounter[j+1] == 1 && valueCounter[j+2] == 1 && valueCounter[j+3] == 1 && valueCounter[j+4] == 1) return (int)straightFlush;
                }
                return (int)flush;
            }
        }

        for(i=2;i<15;++i)
        {
            if(valueCounter[i] == 4) return (int)fourOfAKind;
            if(valueCounter[i] == 3) ++threeKind;
            if(valueCounter[i] == 2) ++pair;
        }

        if(pair == 1 && threeKind == 1) return (int)fullHouse;
        else if(pair == 2) return (int)twoPair;
        else if(pair == 1) return (int)onePair;
    }
    return none;    //none of the above
}

Any help would be much appreciated. Thanks! Below is the valgrind output and program output.

shuffling...
5*3
7*2
10*1
13*1
12*2
13*3
8*1
8*4
4*1
10*4
6*1
13*2
14*3
12*3
5*2
7*4
14*1
6*2
2*2
8*3
7*1
14*4
4*4
10*2
11*3
9*2
11*2
12*1
9*1
7*3
11*1
14*2
3*2
9*4
4*2
8*2
5*1
3*4
10*3
4*3
2*4
6*3
11*4
13*4
5*4
12*4
2*3
3*3
3*1
9*3
6*4
2*1
5*3 7*2 10*1 13*1 12*2
Discard card number 1?yes

13*3 7*2 10*1 13*1 12*2
Discard card number 2?yes

13*3 8*1 10*1 13*1 12*2
Discard card number 3?yes

13*3 8*1 8*4 13*1 12*2
Discard card number 4?no

13*3 8*1 8*4 13*1 12*2
Discard card number 5?no

13*3 8*1 8*4 13*1 12*2
Hand Type: 2

Play Again? yes
==26986== Invalid read of size 8
==26986== at 0x36D5A6F680: std::basic_istream<char, std::char_traits<char> >& std:perator>><char, std::char_traits<char>, std::allocator<char> >(std::basic_istream<char, std::char_traits<char> >&, std::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (in /usr/lib64/libstdc++.so.6.0.16)
==26986== by 0x4012B3: main (Homework5.cpp:53)
==26986== Address 0xffffffffffffffe8 is not stack'd, malloc'd or (recently) free'd
==26986==
==26986==
==26986== Process terminating with default action of signal 11 (SIGSEGV)
==26986== Access not within mapped region at address 0xFFFFFFFFFFFFFFE8
==26986== at 0x36D5A6F680: std::basic_istream<char, std::char_traits<char> >& std:perator>><char, std::char_traits<char>, std::allocator<char> >(std::basic_istream<char, std::char_traits<char> >&, std::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (in /usr/lib64/libstdc++.so.6.0.16)
==26986== by 0x4012B3: main (Homework5.cpp:53)
==26986== If you believe this happened as a result of a stack
==26986== overflow in your program's main thread (unlikely but
==26986== possible), you can try to increase the size of the
==26986== main thread stack using the --main-stacksize= flag.
==26986== The main thread stack size used in this run was 8388608.
==26986==
==26986== FILE DESCRIPTORS: 3 open at exit.
==26986== Open file descriptor 2: /dev/pts/0
==26986== <inherited from parent>
==26986==
==26986== Open file descriptor 1: /dev/pts/0
==26986== <inherited from parent>
==26986==
==26986== Open file descriptor 0: /dev/pts/0
==26986== <inherited from parent>
==26986==
==26986==
==26986== HEAP SUMMARY:
==26986== in use at exit: 29 bytes in 1 blocks
==26986== total heap usage: 3 allocs, 2 frees, 82 bytes allocated
==26986==
==26986== 29 bytes in 1 blocks are definitely lost in loss record 1 of 1
==26986== at 0x4A06FC7: operator new(unsigned long) (vg_replace_malloc.c:261)
==26986== by 0x36D5AA2E48: std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (in /usr/lib64/libstdc++.so.6.0.16)
==26986== by 0x36D5AA3BBA: std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) (in /usr/lib64/libstdc++.so.6.0.16)
==26986== by 0x36D5AA3C9F: std::string::reserve(unsigned long) (in /usr/lib64/libstdc++.so.6.0.16)
==26986== by 0x36D5A6F820: std::basic_istream<char, std::char_traits<char> >& std:perator>><char, std::char_traits<char>, std::allocator<char> >(std::basic_istream<char, std::char_traits<char> >&, std::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (in /usr/lib64/libstdc++.so.6.0.16)
==26986== by 0x400FB6: main (Homework5.cpp:37)
==26986==
==26986== LEAK SUMMARY:
==26986== definitely lost: 29 bytes in 1 blocks
==26986== indirectly lost: 0 bytes in 0 blocks
==26986== possibly lost: 0 bytes in 0 blocks
==26986== still reachable: 0 bytes in 0 blocks
==26986== suppressed: 0 bytes in 0 blocks
==26986==
==26986== For counts of detected and suppressed errors, rerun with: -v
==26986== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 2 from 2)
Segmentation fault (core dumped)

trying to compile yours program. where is "prototypes.h"?

Code:

while(i<15) {valueCounter[i] = 0, suitCounter[i] = 0; ++i;}

How many suits do you have?

TBH, you need some constants in your code, such as

Code:

const int maxRank = 13;
const int maxSuit = 4;
const int deckSize = maxRank * maxSuit;
const int handSize = 5;

Use these to set your array sizes, and control your loop limits.
Though having said that, have you considered replacing your arrays with std::vector ?

Code:

        sleep(1);
        srand(time(NULL));
        j = rand()%51;

If you just called srand() once at the start of main, then you wouldn't need to sleep (and make your program unnecessarily slow) just to get different results from rand()

Code:

void deck::shuffle()
{
    card temp;
    int i,j;
    cout<<"shuffling..."<<endl;
    for(i = 0; i < 52; ++i)
    {
        temp = cards[i];
        sleep(1);
        srand(time(NULL));
        j = rand()%51;
        cards[i] = cards[j];
        cards[j] = temp;
    }
}

This is very very wrong. First the idea that you would have a sleep(1) in the loop ... bizarre. You're forcing it to take 52 seconds to complete. Get rid of the sleep.

I can only assume that you put it there after realizing that without it, time(NULL) will return the same value 52 times in a row (since the loop will complete in well under a second). But the sleep is not the right solution.

The proper thing to do is to simply move the srand(time(NULL)) out of the loop. Put it in deck::deck(). In general, it should only be executed once in the entire run of the program. (So normally you'd also have a static bool to determine if it's already been done so that if another deck was created it won't be done again, but you don't need to do that here.)

Also, your shuffle algorithm is wrong. It should be

Code:

for (int i = 51; i > 0; i--) {
    int j = rand() % (i + 1);
    swap(card[i], card[j]);
}

Originally Posted by oogabooga

The proper thing to do is to simply move the srand(time(NULL)) out of the loop. Put it in deck::deck(). In general, it should only be executed once in the entire run of the program. (So normally you'd also have a static bool to determine if it's already been done so that if another deck was created it won't be done again, but you don't need to do that here.)

Why not just put it in main()? It doesn't make sense for deck to be responsible for seeding the RNG imo.

This is a clear sign that you are using the wrong loop type:

Code:

    bool playAgain = true;

    //play game
    while(playAgain == true)
    {

Instead, you should be using a do..while loop there.

You also don't need the "== true" in there, as playAgain is already a boolean. Afterall, you wouldn't do this would you?:

Code:

 while((playAgain == true) == true)

Originally Posted by iMalc

This is a clear sign that you are using the wrong loop type:
Instead, you should be using a do..while loop there.

A do while loop would be the best loop type here because it communicates the intention of running the loop at least once, but then a while loop isn't quite wrong either since it is good practice to initialise playAgain anyway, and it is thus easy to see that the loop will run at least once. However, if you were to modify this code to be in a function, say named askPlayAgain:

Code:

cout<<endl<<"Play Again? ";
cin>>answer;
if(answer != "yes") playAgain = false;

Then you can dispense with the playAgain variable in main and have a neat:

Code:

do
{
    // ...
}
while (askPlayAgain());