11
\$\begingroup\$

I have implemented to following generic factory in C++11 using smart pointers and I would like to get some feedback about it.

(Please note that I can't use C++14)

Due to company policies I have to change the names of the business objects.

class Car{};
class Taxi : public Car{};
class Police : public Car{};

class CarFactory {

public:
    enum CarName {TaxiName, PoliceName};

    std::unique_ptr<Car> getCar(CarName sa) {
        if (d_carMap.find(sa) == d_carMap.end()) {
            d_carMap[sa] = std::move(createCar(sa));
        }
        return std::move(d_carMap[sa]);
    }

private:
    std::unordered_map<CarName,
                       std::unique_ptr<Car>,
                       std::hash<int>> d_carMap; //hash is for enum

    std::unique_ptr<Car> createCar(CarName sa) {

        if (sa == CarName::TaxiName){
          return std::unique_ptr<Car>(new Taxi());
        } else if (sa == CarName::PoliceName) {
          return std::unique_ptr<Car>(new Police());
        }

        return nullptr; //no car found for provided argument.
    }

};
  1. Which of the moves are actually needed here?
  2. What happens to the unique_ptrs, if this factory is used in a multi threading environment?
  3. Is std::unique_ptr<Car>(new Taxi()); really the best way of instantiating a smart pointer in C++11 (which doesn't have make_unique)?
  4. Should the enum be a class, even if I can't use std::hash on it?
  5. Where would you define the enum? In a separate header file? In the same header, outside of the class (but in a namespace)?
  6. How should the unit test look like?

I wrote the following test which actually fails on the indicated line:

TEST(CarFactoryTest, retrieveTaxi) {

    CarFactory sf;

    auto Car = sf.getCar(CarFactory::CarName::TaxiName);
    if (!Car) {
        FAIL();
    }

    if (dynamic_cast<Taxi*>(Car.get()) == nullptr) {
        FAIL();
    }

    //this time get it from the map
    Car = sf.getCar(CarFactory::CarName::TaxiName);
    if (!Car) {
        FAIL(); //Test fails here!
    }

    if (dynamic_cast<Taxi *>(Car.get()) == nullptr) {
        FAIL();
    }
}
\$\endgroup\$
7
\$\begingroup\$

1) Which of the moves are actually needed here?

Since there are only two moves, let's have a look at them:

std::unique_ptr<Car> getCar(CarName sa) {
    if (d_carMap.find(sa) == d_carMap.end()) {
        d_carMap[sa] = std::move(createCar(sa)); // (1)
    }
    return std::move(d_carMap[sa]);              // (2)
}

Only the latter is necessary. In (1), the expression createCar(sa) is a prvalue expression and therefore a rvalue. In (2), d_carMap[sa] is a lvalue expression, since operator[] returns a lvalue reference.

Or: if you try to move something that has a name, or if the function returns T&, you need std::move.

2) What happens to the unique_ptrs, if this factory is used in a multi threading environment?

Undefined behaviour. There's a race condition if one uses std::move on the same unique_ptr twice: the move assignment gets evaluated half-through twice, you end up with two unique_ptr owning the same memory, and two calls to delete. Note that the std::unorded_map isn't thread-safe either.

3) Is std::unique_ptr(new Taxi()); really the best way of instantiating a smart pointer in C++11 (which doesn't have make_unique)

No. Use make_unique, but make sure to put it in your own namespace, not std. That way, you can simply replace your version with the C++14 one later.

4) Should the enum be a class, even if I can't use std::hash on it?

That doesn't really make sense. Either you have an enumeration, or you don't. Unless you mean a scoped enumeration (enum class). And yes, scoped enums are usually better. But that depends on 5).

And you can use std::hash on a scoped enum, even in C++11, since you're allowed to specialize std::hash for your type. It's one of the exceptions where you may mingle in the std namespace.

5) where would you define the enum? In a separate header file? In the same header, outside of the class (but in a namespace)?

The "namespace" part isn't necessary if you use a scoped enum. The place of the enum depends on its use. If it's only used together with this class, it's reasonable to keep it in the same header. If CarName can get used without CarFactory, it's probably a better idea to put it into another unit. But that depends on your use case.

6) How should the unit test look like? I wrote the following test which actually fails on the indicated line.

And it should fail. You moved the ownership of whatever d_carMap[sa] pointed to outside. However, you didn't remove the actual std::unique_ptr from your map. It's still there, being the same as a nullptr. Therefore, your d_carMap.find() yields an iterator:

// creates a ClownCar, stores it in d_carMap
// in a pair (ClownCar, <some unique_ptr>) and
// immediately moves from the <unique ptr>
// thus d_carMap contains (ClownCar, nullptr) 
// after the call.
auto myCar = factory.getCar(ClownCar); 

// find(ClownCar) finds (ClownCar, nullptr)
auto myNullCar = factory.getCar(ClownCar); 

// whoops

So the problem is that you don't get rid of the nullptr std::unique_ptr in your map. And your test have shown that, so they're actually helping.

Overall I think that you've abstracted the code a little bit too much. It's intend isn't clear, since the documentation is missing. Also, the indentation isn't uniform, but that's a minor problem.

\$\endgroup\$
  • \$\begingroup\$ Thanks Zeta for the answer. 1) I am not even sure why the second move is necessary, I thought "In the case of return std::move(foo); the move is superfluous" \$\endgroup\$ – user695652 Jul 8 '16 at 22:05
  • \$\begingroup\$ @user695652: Depends. See stackoverflow.com/q/14856344/1139697. d_carMap[sa] isn't eligible for copy elision and it's an lvalue (reference). A move is necessary here. \$\endgroup\$ – Zeta Jul 9 '16 at 14:28
4
\$\begingroup\$

Your factory doesn't really make sense in its current form. You might want to consider using a shared_ptr instead of a unique_ptr.

Looking at your code, you check your map to see if there is an instance of the car in it. If there isn't then you create one, add it to the map and then return it. If there is, then you return the pointer from the map.

std::unique_ptr<Car> getCar(CarName sa) {
    if (d_carMap.find(sa) == d_carMap.end()) {
        d_carMap[sa] = std::move(createCar(sa));
    }
    return std::move(d_carMap[sa]);
}

The problem is, that when you return the pointer from the map, ownership of the pointer is moved to the returned unique_ptr. This nulls out the pointer stored in the map. If you want both the map + the returned pointer to point at the same instance then you really need to look at using a shared_ptr. Alternately, you could remove the map entirely and create a new instance for each request.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your answer, the map is exactly the problem, I agree, but the Police and Taxi objects are expensive to instantiate so I really want to store them in a map. I was reluctant to use shared_ptr because according to Scott Meyers a Factory should return unique_ptr, which still give the user the ability to cast it to a shared_ptr. But am I right that if I want to store the created objects in my map (and dont want to use raw pointers) then shared_ptr is the way to go? \$\endgroup\$ – user695652 Jul 8 '16 at 22:01
  • 2
    \$\begingroup\$ @user695652 Only one unique_ptr can point at any particular instance, so yes you will need to use a shared_ptr if you want both the map and the returned instance to point at the same thing. If you're allowing multi-threaded access then even using shared_ptr you are going to have issues with the creation / map storage as described by zeta's answer, so you would need to use some kind of locking. You'd also need to be aware that all of your pointers returned from the create method pointed at the same instance, which again could cause issues if you are accessing it from multiple threads. \$\endgroup\$ – forsvarir Jul 8 '16 at 22:07
  • 1
    \$\begingroup\$ @user695652 Whether your factory returns a unique pointer or a shared pointer depends on whether or not it is safe to give other classes a pointer to the same instance. For example, if the classes getting the Cars from the CarFactory believe they are the only owner of the Car and think they can do what they like (e.g. change the speed of the Car, add/remove fuel), they're going to get a nasty shock when another class has changed the state of the Car behind their backs. It's worth noting that immutable objects can always be shared without fear because their state cannot be changed. \$\endgroup\$ – Pharap Jul 9 '16 at 9:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.