Why in my simple C++ code output is -1717986920?

Question

I just started C++ learning. I wanted to create a class.

The code is:

#include <iostream>
using namespace std;


class MyClass {
public:
    int a;
    int b;

    MyClass(int nA, int nB) {
        int a = nA;
        int b = nB;
    }

    int add() {
        int output = a + b;
        return output;
    }
};

void main() {
    system("color 0a");

    int oa = 2;
    int ob = 3;

    MyClass mc1(oa, ob);

    cout << mc1.add();
}

The output should be 5, but it's somehow -1717986920

What did I do wrong?

`int a = nA;` assigns nA to a *local* variable with automatic storage duration which *shadows* the field `a`. You need to be more mindful of when you *actually* want to declare a variable, and not place declarations arbitrarily without consideration. — nanofarad, Apr 23 '21 at 14:08
Read about member variables and function-local variables, and about lexical scope and name shadowing, in your favourite C++ book. — molbdnilo, Apr 23 '21 at 14:08
Once again, a problem that would be solved if people used constructor initialization sections. — sweenish, Apr 23 '21 at 14:09
pay attention to compiler warnings: https://godbolt.org/z/MnsKsMchM. Its is easy to write wrong code that compiles without compiler error. Writing wrong code without warnings is a little harder — 463035818_is_not_an_ai, Apr 23 '21 at 14:23
When you get a really weird number convert it into hexadecimal. Sometimes, not this time though, [that number is the program trying to tell you something.](https://en.wikipedia.org/wiki/Magic_number_(programming)#Debug_values). Maybe if you had the values of `a` and `b`, something you really should print out to help you understand how you got the wrong result for `a+b` you would see a special code for uninitialized value. — user4581301, Apr 23 '21 at 14:40
When you get a bad result, start printing out variables that lead to the wrong result. The closer you can get the the beginning of the problem, the easier it is to find the mistake. — user4581301, Apr 23 '21 at 14:48
There should be a tool that came with your compiler called a debugger. Because programmers like joke names and acronyms you might have to look up what the debugger is named, but once you have that figures out, you can use the debugger to run the program at super-slow speed and view the variables so you can see what the program does as it does it. Awesomely helpful in finding where thigs initially went wrong. As soon as you see program doing something you don't expect, like storing the wrong value or taking the wrong path, you've probably found a bug. — user4581301, Apr 23 '21 at 14:48

score 3 · Answer 1 · answered Apr 23 '21 at 14:08

3

MyClass(int nA, int nB) {
    int a = nA;
    int b = nB;
}

is just setting value of the unused local variables and leaving the member variables uninitialized.

You can use member initializer list to initialize member variables:

MyClass(int nA, int nB) : a(nA), b(nB) {
}

In this case you can set values of the member variables in the constructor instead:

MyClass(int nA, int nB) {
    a = nA;
    b = nB;
}

answered Apr 23 '21 at 14:08

MikeCAT

73,922
11
45
70

1

One of these is bad practice. I recommend only recommending the use of the initialization section. I get correcting the error, but you're still left with a bad practice. – sweenish Apr 23 '21 at 14:11

score 2 · Answer 2 · answered Apr 23 '21 at 14:09

2

MyClass(int nA, int nB) {
    int a = nA;
    int b = nB;
}

You're defining new versions of a and b, local to the constructor. Do one of these instead:

MyClass(int nA, int nB) {
    a = nA;
    b = nB;
}

MyClass(int nA, int nB)
: a(nA), b(nB)
{
}

Either will fix your problem.

Also, I strongly encourage you to always auto-initialize variables like this:

class MyClass {
public:
    int a = 0;
    int b = 0;
};

answered Apr 23 '21 at 14:09

Joseph Larson

8,530
1
19
36

But one of them is bad practice. I recommend only recommending the use of the initialization section. – sweenish Apr 23 '21 at 14:10
1

@sweenish He's a newbie. I suspect the second one is confusing. The first is easier to understand, and there are times that you can't use initialization lists. – Joseph Larson Apr 23 '21 at 14:11
1

Newbies need to learn to do it right, and this is not one of those rare times. – sweenish Apr 23 '21 at 14:11
1

@sweenish Which is why I showed both ways of doing it. Newbies can only learn so much in one sitting. – Joseph Larson Apr 23 '21 at 14:13
Exactly, so show them the better way. – sweenish Apr 23 '21 at 14:14
@sweenish Which I did. – Joseph Larson Apr 23 '21 at 14:20
Alongside an inferior method which can let subtle bugs in. You're the one who claimed limited capacity on the learner's part. If it's limited, showing one thing is better than two, and if you're picking one, it should be the right way. I'm just tired of seeing this issue on SO. Users are going to get religious over vectors at any opportunity, but not this? – sweenish Apr 23 '21 at 14:24
@sweenish I personally believe when helping someone learn to code, that when they bring a piece of code to you that is broken, showing them the least number of changes to fix it is helpful. So that's what I did. I remember when I was learning C++ (almost 30 years ago), and I struggled with some of the concepts -- including initializer lists. Showing it as a learning progression is, in my opinion, useful. This is my last response in what is a horribly stupid argument. – Joseph Larson Apr 23 '21 at 14:28
i don't think its horribly stupid. Showing both, immediately brings subtle details to the table, like how initializations differes for an `int` and eg a `std::string` member. If a newbie knows the initializer list they can do almost anything with it and it will be right thing most of the time. – 463035818_is_not_an_ai Apr 23 '21 at 14:37
That's all well and good, but a "strongly encourage" doesn't change practices. I have taught C++, and showing the minimal changes with a recommendation gets the minimal changes submitted 9 times out of 10. At the end of the day, I place the blame on wherever OP is learning C++ from; it's flawed. I wouldn't say this is a stupid argument either. I'm not arguing about syntactic sugar, there are real behavioral differences. I didn't say you should dive behind the hood, just only show the better thing since the first never gets past review in industry it's that egregious. – sweenish Apr 23 '21 at 14:42
But if this site wants to be religious about things like vectors and smart pointers, this should be on that list. – sweenish Apr 23 '21 at 14:43

Adrian Mole · Accepted Answer · 2021-04-23T14:19:35.860

In your constructor, you declare two local variables, a and b, and assign the given arguments' values to those – leaving the member variables of the same name untouched (and, thus, uninitialized). Remove the 'declaration' part of the assignments:

    MyClass(int nA, int nB) {
        a = nA;
        b = nB;
    }

Or, for a terser/better version, use an initializer list:

    MyClass(int nA, int nB) : a{nA}, b{nB} {}

You can readily spot mistakes like this by enabling all compiler warnings; for example, the clang-cl compiler gives two messages like the following:

warning : declaration shadows a field of 'MyClass' [-Wshadow]

Note, also, that your void main() is not valid C++ (although it's acceptable in some versions of C); this should be int main() and the function should (normally) end with a return 0; statement.

It's not just terser, it's best practice. The two methods behave differently. — sweenish, Apr 23 '21 at 14:13

score 2 · Answer 4 · edited Apr 23 '21 at 14:23

First, make sure your member variables are initialized for the default case

int a { 0 };

Second, in constructor use member initialization notation

Third, avoid creating unnecessary temp variables in add(). You should also make add() const since calling add() doesn't modify any member variables.

class MyClass {
public:
    int a { 0 };
    int b { 0 };
    
    MyClass(int nA, int nB) : a(nA), b(nB) {  }
    
    int add() const { return a + b; }
};

Why in my simple C++ code output is -1717986920?

4 Answers4