12

I have a class MyClass that owns an instance of some DataProvider class and has a getter for this.

For the sake of Dependency Injection I would prefer to have a getter and a setter. Additionally the DataProvider should be contained in a std::unique_pointer:

#include <memory>
#include <iostream>

class DataProvider
{
public:
    DataProvider() {}
    virtual ~DataProvider() {}
    /* stuff */

private:
    /* more stuff */

};

class MyClass
{
public:
MyClass() {}

    virtual inline const DataProvider &getDataProvider() const
    {
        return *data;
    }
    void setDataProvider(std::unique_ptr<DataProvider> newData)
    {
        data = std::move(newData);
    }

private:
    std::unique_ptr<DataProvider> data;
};

I've read this: How do I pass a unique_ptr argument to a constructor or a function?

But it does not cover the getter part. Is this (above) the right way to do this? What are alternatives to think of?

Community
  • 1
  • 1
TobiMcNamobi
  • 4,687
  • 3
  • 33
  • 52
  • full example would be better then code snippets – BЈовић Apr 15 '13 at 08:51
  • [Works fine here](http://ideone.com/enarAS). What vtable? We don't have any virtual functions. – Joseph Mansfield Apr 15 '13 at 08:59
  • @sftrabbit You're right, I had to add some more info about DataProvider and the way that it is broken in the end. – TobiMcNamobi Apr 15 '13 at 09:14
  • Does the default constructor of `DataProvider` set `xyz`? – Joseph Mansfield Apr 15 '13 at 09:16
  • DataProvider has a setter for xyz. edit: No, wait ... yes the default constructor sets it to 0 AND there is a setter for it. – TobiMcNamobi Apr 15 '13 at 09:18
  • @user1916893: "*the default constructor sets it to 0 AND there is a setter for it*" I don't see any of that in the code you provided. So how can we be expected to know things you haven't shown us? – Nicol Bolas Apr 15 '13 at 09:30
  • Sorry, DataProvider is 1500+ lines of legacy code. I have to extract the lines which I think are important. – TobiMcNamobi Apr 15 '13 at 09:33
  • 2
    @user1916893 Okay, extract the lines you think are important and make a new small program using them. When you experience the same problem, post it here. For all we know, the problem is in the 1500 lines of legacy code. – Joseph Mansfield Apr 15 '13 at 09:34
  • Status update: In a small test project everything works fine. At least it seems to me that using a unique_ptr together with getter and setter as seen above is OK. I will keep you informed. – TobiMcNamobi Apr 15 '13 at 11:15
  • Ah, the solution is too embarrassing. I rebuilt the whole solution (production). Now everything is fine. Thank you all for your time and effort, I learned a lot this morning. – TobiMcNamobi Apr 15 '13 at 11:30
  • Be careful with this approach. If `MyClass` is destroyed before a class using the getter, you will have problems. Consider using a `shared_ptr`. – Rotsiser Mho Feb 27 '14 at 21:36

2 Answers2

11

As you wrote

virtual inline const DataProvider &getDataProvider() const
{
    return *data;
}
void setDataProvider(std::unique_ptr<DataProvider> newData)
{
    data = std::move(newData);
}

is perfectly OK. In the setter the class gets ownership of the DataProvider instance and never let go (as far as seen here).

A full example has been given by sftrabbit here: http://ideone.com/enarAS

TobiMcNamobi
  • 4,687
  • 3
  • 33
  • 52
  • 1
    Can you explain this whole thing `virtual inline const DataProvider &getDataProvider() const` (the use of `virtual`, `inline` and the two `const`) for a newbie? :) – valentin Apr 05 '14 at 01:21
  • 2
    @toggy `getDataProvider` is the name of the method, that's an easy one I guess. `const` in general means "there is something constant here, a variable which has a value that cannot be changed" (it can be bypassed but in this comment I assume that `const` means "not changeable"). The second `const` just means that in this method no class member will be changed. The `this` pointer itself is `const`. The first `const` is part of the return type `const DataProvider&` - a reference to a DataProvider object and this object cannot be changed. – TobiMcNamobi Apr 07 '14 at 07:28
  • 2
    @toggy A method declared as `virtual` is best explained with an example: Say you have a base class A and a class B deriving from A. A has a public method f(). So you can call f() on objects of type A and on objects of type B. Now B has a method f(), too. Consider a pointer c to an object of type A: `A* c;`. If c actually points to an object of type B and you write 'c->f();', the f() of A will be called *unless* f() is virtual in which case the f() of B will be called. That's the difference. – TobiMcNamobi Apr 07 '14 at 07:36
  • 2
    @toggy 'inline' means that while building your project the optimizer shall *prefer*(!) to substitue the method body at each place where the method is called. In fact I normally don't use this keyword and let the compiler decide for itself. Anyway I found a good explanation here: http://en.cppreference.com/w/cpp/language/inline – TobiMcNamobi Apr 07 '14 at 07:40
  • I didn't mean 'that' newbie x) I just wanted to know why they were 2 const keywords and why it was useful to use virtual and inline here. Thanks :p – valentin Apr 07 '14 at 11:18
  • In the getter method you're returning a reference to a temporary object, is that _safe_? – Kill KRT Mar 09 '19 at 05:35
  • 1
    @KillKRT `*data` is casted to a reference to a `member`, not a temporary. So, yes, it's safe. – TobiMcNamobi Mar 11 '19 at 08:09
1

While I am not sure if this really is what you need this is a starting point:

const DataProvider &getDataProvider() const
{
    return *data.get();
}
Tomek
  • 4,554
  • 1
  • 19
  • 19