Constructor storing different than what's being passed in

Question

I'm calling my constructor, but when I cout the values before they go in, they are correct, when I cout the values while in the constructor, I get nonsense and blanks.

Function:

Play parse(string toParse){
    vector<string> tokens;
    string play = toParse;
    string oName, dName;
    int x, quarter, minutes, down, yardstogo, startloc, playdesc;
    for(int y=0; y<10; y++){
        x = toParse.find(",");
        tokens.push_back(toParse.substr(0,x));
        toParse = toParse.substr(x+1);
    }
    stringstream convert(tokens[1]);
    convert >> quarter;
    convert.str(tokens[2]);
    convert >> minutes;
    convert.str(tokens[6]);
    convert >> down;
    convert.str(tokens[7]);
    convert >> yardstogo;
    convert.str(tokens[8]);
    convert >> startloc;
    playdesc = findPlay(tokens[9]);
    cout << "quarter: " << quarter << endl << "oteam: " << tokens[4] << endl;
    Play a(quarter, minutes, tokens[4], tokens[5], down, yardstogo, startloc, playdesc, play);
    return a;
}

Constructor:

Play::Play(int m_quarter, int m_minutes, string oTeam, string dTeam, int down, int yardToGO, int startLoc, int playDesc, string wholePlay)
{
    m_quarter = m_quarter;
    cout << m_quarter << endl;
    m_minutes = m_minutes;
    oTeam = oTeam;
    cout << oTeam << endl;
    dTeam = dTeam;
    down = down;
    yardToGO = yardToGO;
    startLoc = startLoc;
    playDesc = playDesc;
    wholePlay = wholePlay;
}

For example, the function says "quarter: 1 oteam: DAL" while the constructor says "quarter: -947800344 oteam: ".

Thanks.

#ifndef PLAY_H_INCLUDED
#define PLAY_H_INCLUDED
#include <string>

class Play
{
private:
    int m_quarter;
    int m_minutes;
    std::string oTeam;
    std::string dTeam;
    int m_down;
    int m_yardToGO;
    int m_startLoc;
    int playDesc;
    std::string wholePlay;
public:
    int getQuarter();
    int getMinutes();
    std::string getoTeam();
    std::string getdTeam();
    int getDown();
    int getYard();
    int getStartLoc();
    int getPlayDesc();
    std::string getwholePlay();
    Play(int m_quarter, int m_minutes, std::string oTeam, std::string dTeam, int down, int yardToGO, int startLoc, int playDesc, std::string wholePlay);
    ~Play();
    Play parse(std::string toParse);
    std::string findPlay(std::string playDesc);
};

#endif // PLAY_H_INCLUDED

`m_quarter = m_quarter;` won't do anything useful. Is that your exact constructor? — aschepler, Sep 16 '13 at 21:07
The value of quarter was passed in under the name m_quarter, and then I want to set my m_quarter value which is part of my object. I'm posting my class if that clears anything up. — Will Nasby, Sep 16 '13 at 21:12
@WillNasby Are you expecting the compiler to guess which one of the two things named m_quarter to use? It may be obvious to you what you meant, but you have actually told the compiler to do something quite different from what you intended. Change one of the names. — john, Sep 16 '13 at 21:13
In fact, you are doing the constructor thing kind of wrong. You should be using member initialization instead of setting them in the constructor body. This might help: http://stackoverflow.com/questions/7665021/c-member-initialization-list — Zan Lynx, Sep 16 '13 at 21:18

score 1 · Answer 1 · answered Sep 16 '13 at 21:15

You are setting the variables to themselves:

Play::Play(int m_quarter, int m_minutes, string oTeam, string dTeam, int down, int yardToGO, int startLoc, int playDesc, string wholePlay)
{
    m_quarter = m_quarter; // setting the same variable to itself
    cout << m_quarter << endl;
    m_minutes = m_minutes; // same
    oTeam = oTeam; // same
    cout << oTeam << endl;
    dTeam = dTeam; // same
    down = down; // same
    yardToGO = yardToGO; // same
    startLoc = startLoc; // same
    playDesc = playDesc; // same
    wholePlay = wholePlay; // same
}

Should be:

Play::Play(int quarter, int minutes, const string& offense, const string& defense, int dwn, int ytg, int start, int desc, int play) 
 : m_quarter(quarter), m_minutes(minutes), oTeam(offense), dTeam(defense), down(dwn), yardToGo(ytg), startLoc(start), playDesc(desc), wholePlay(play)
{
}

Also, if you are going to denote member variables with the m_ prefix, be consistent and do it with all of them.

BigTailWolf · Answer 2 · 2013-09-16T21:48:12.713

Your constructor seems have some problem.

Your member's name should not be the same name as parameters. And using initialization instead of assignment. See Scott Meyer's "Effective C++" Item 12.

Like this:

Play::Play(int quarter, int minutes, const string& offense, 
           const string& defense, int dwn, int ytg,
           int start, int desc, int play) 

         : m_quarter(quarter)
         , m_minutes(minutes)
         , oTeam(offense)
         , dTeam(defense)
         , down(dwn)
         , yardToGo(ytg)
         , startLoc(start)
         , playDesc(desc)
         , wholePlay(play)
{
}

Also, you don't have to process your input that complex. Just read from you stringstream and throw commas into a garbage string variable. (You must to be very sure of this format, otherwise it might be cause some error)

Such as your parsing string "toParse":

int quarter, minutes;
string oName, dName;
int down, yardstogo, startloc, playdesc;
string wholePlay;
string comma;

stringstream ss(toParse);
toParse >> quarter >> comma >> minutes >> comma
        >> oName >> comma >> dName >> comma
        >> dwn >> comma >> yardstogo >> comma
        >> startloc >> comma >> playdesc >> comma
        >> wholePlay;

return Play(quarter, minutes, oName, dName, dwn, 
            yardstogo， startloc, playdesc, wholePlay);

score 0 · Answer 3 · answered Sep 16 '13 at 21:13

Make sure that your constructor arguments have names that differ from your members, so for instance

Play::Play(int quarter, //etc..
{
    m_quarter = quarter;
    // etc..
}

Or even better, use member initializers where possible:

Play::Play(int quarter, //etc..
    :m_quarter( quarter), // etc...
{
}

Strictly speaking, when you do the latter, the names could be the same again, but in general a m_-prefix indicates a member variable, not an argument.

score 0 · Answer 4 · answered Sep 16 '13 at 21:31

As an aside, If you insist on not using initializer lists because you want to be different and for some reason don't like Scott Meyer, you can use the this pointer:

Play::Play(int m_quarter,
{
    this->m_quarter = m_quarter;
    // etc
}

but I would recommend using the initializer list instead.

Constructor storing different than what's being passed in

4 Answers4