C++ initialising a class pointer through a function

Question

I have a program that has a single instance of the class class A and many instances of the class class B, where every instance of class B has a pointer to a single instance of class A. I thought I could initiate them in main() and then pass the address of the single instance of class A to all instances of class B. I want to know if this is correct, I have been looking at inheritance but from my understanding (which is often wrong) if you inherit another class then that class gets initiated every time so creates a many to many relationship whereas, I want a one to many. I have attached some code. Any suggestions would be greatly appreciated.

// C/C++ standard library
#include <vector>
#include <iostream>
#include <cstdlib>

using namespace std;

class A {
public:
    double get_value(void) {
        return value;
    }
private:
    double value;
};


// Forward declare A if split over files
class B {
public:
    void assign_pointer(A class_a_to_assign) {
        class_a = &class_a_to_assign; // assign the pointer the address to point to
    }
    void update_my_value(void) {
        value_b += class_a->get_value();
    }

    double get_value(void) {
        return value_b;
    }
private:
    double value_b = 0.1;
    A* class_a; // pointer to class A
};

int main() {
    cout << "hello world" << endl;
    // create 2 instances of B there could be thousands of these tho.
    B b1;
    B b2;
    // create 1 instance of A
    A a1;

    // Now I want both instances of the B class to point to the one instance of A
    b1.assign_pointer(a1);
    b2.assign_pointer(a1);

    // THen do stuff with B so that if any changes occur in A, then they can be automatically updated in class B through the pointer

    b1.update_my_value();
    b2.update_my_value();

    cout << b1.get_value() << " and " << b2.get_value() << endl;
    return 0;
}

An improvement would be to create a1 before all the B objects, to ensure it outlives them, since they depend on it. — juanchopanza
– juanchopanza, Commented May 28, 2018 at 5:07
Inheritance isn't many-to-many, it's just a one-to-one relationship for each object. (Also, a forward declaration of A wouldn't suffice for B's definition, and your example reads from the uninitialized A::value.) There's nothing else (obviously) wrong here (even if significant improvements could be made)--is that your whole question? — Davis Herring
– Davis Herring, Commented May 28, 2018 at 5:08
Another one: make sure B can only be instantiated from an A*by doing it in the constructor: explicit B(A* a) : class_a(a) {}. — juanchopanza
– juanchopanza, Commented May 28, 2018 at 5:16

bipll · Accepted Answer · 2018-05-28 05:26:13Z

First of all, there's a serious issue in your code:

void assign_pointer(A class_a_to_assign) {
    class_a = &class_a_to_assign; // assign the pointer the address to point to
}

Here, class_a_to_assign is a by-value function argument, it is approximately the same in terms of lifetime as any function-local variable. In other words, as soon as control leaves method's scope, class_a becomes a dangling pointer (a pointer to a local object that does no more exist). A quickfix is simple and straightforward:

void assign_pointer(A &class_a_to_assign) {
    class_a = &class_a_to_assign; // assign the pointer the address to point to
}

The difference is but a single character — the ampersand in function's argument declaration turns it from a temporary value into a reference to a more long-lived object.

Next, if you have a single object of class A have you considered making it a singleton? This way, instances of B would not even need to keep that pointer, A itself manages the instance. There's a lot been said out there about designing a singleton class, a crude and naive implementation is something like:

class A {
    A(); // note it's private
public:
    int getValue() const;
    static A &instance() {
        static A retVal;
        return A;
    };
};

class B {
public:
    void update_my_value(void) {
        value_b += A::instance().get_value();
    }
};

int main() {
    A::instance(); // to warmup the static instance before any users are created
    B b1; // no assign_pointer is needed as A is a Singleton
    B b2; // and every B always knows where to find it
}

R Sahu · Accepted Answer · 2018-05-28 05:23:47Z

Your intent to create only one instance of A and use a pointer to that instance of A is violated in the function below.

void assign_pointer(A class_a_to_assign) {
    class_a = &class_a_to_assign; // assign the pointer the address to point to
}

Since you are accepting class_a_to_assign by value, you have two problems:

You are creating a new instance of A every time you call the function.
You are storing a pointer to a temporary object. The pointer becomes an invalid pointer as soon as the function returns.

You can fix both problems by making the argument a reference type.

void assign_pointer(A& class_a_to_assign) {
    class_a = &class_a_to_assign; // assign the pointer the address to point to
}

Also, as has been mentioned in comments, it will be better to create the A object before the B objects since you want the A to outlive the B objects.

int main() {
    cout << "hello world" << endl;

    // create 1 instance of A
    A a1;

    // create 2 instances of B there could be thousands of these tho.
    B b1;
    B b2;

    ...
}

user1118321 · Accepted Answer · 2018-05-28 05:23:58Z

What you've described sounds a lot like a singleton. One way this is often handled is to have the singleton class include a static method that returns the single instance of the class. So it would look something like this:

class A {
    public:
        A() { /* constructor */ }
        ~A() { /* destructor */ }
        static A* getInstance();
        double get_value(void) {
            return value;
        }
    private:
        double value;
};

The static method would be in the implementation (.cpp) file look something like this:

static A gSharedInstance;

static A* A::getInstance()
{
    return &gSharedInstance;
}

This constructs an A at static initialization time and when the above method is called, it returns a pointer to that static shared instance. Now wherever you want to use an A, you can simply do:

A* sharedA = A::getInstance();
double value = sharedA->getValue(); // Or whatever you need from it.

Note that a singleton is essentially a global variable, and global variables have some issues. (For example, where is its value member set in this case?) But if the class represents some truly unique resource (such as the only microphone on a device, for example), then it's worth having a singleton to represent it. In that case, it may be best to make it read-only so that its state can't be set in many different places. Also, be aware that you may need to add code to deal with multiple threads accessing it at the same time.

Collectives™ on Stack Overflow

C++ initialising a class pointer through a function

3 Answers 3

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related