Beep-beep I'm a car factory

Question

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();
    }
}

Answer 1

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.

Answer 2

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.