3

Suppose I want to write factory method that is supposed to allocate heterogeneous objects on the heap and return them to the caller. I am thinking of designing the API like this:

bool MakeEm(auto_ptr<Foo>& outFoo, auto_ptr<Bar>& outBar) {
  ...
  if (...) {
    return false;
  }
  outFoo.reset(new Foo(...));
  outBar.reset(new Bar(...));
  return true;
}

This allows a caller to do this:

auto_ptr<Foo> foo;
auto_ptr<Bar> bar;
MakeEm(foo, bar);

My question is: "Is this idiomatic? If not, what is the right way to do this?"

The alternative approaches I can think of include returning a struct of auto_ptrs, or writing the factory API to take raw pointer references. They both require writing more code, and the latter has other gotchyas when it comes to exception safety.

Dilum Ranatunga
  • 13,254
  • 3
  • 41
  • 52

5 Answers5

4

Asking of something is idiomatic can get you some very subjective answers. In general, however, I think auto_ptr is a great way to convey ownership, so as a return from a class factory - it's probably a Good Thing. I would want to refactor this, such that

  1. You return one object instead of 2. If you need 2 objects that are so tightly coupled they cannot exist without each other I'd say you have a strong case for is-a or has-a refactoring.
  2. This is C++. Really ask yourself if you should return a value indicating success, forcing the consumer of your factory to have to check every time. Throw exceptions or pass exceptions from the constructors of your classes in the factory. Would you ever want to be OK with false and try to operate on uninitialized auto_ptr's?
Josh
  • 6,155
  • 2
  • 22
  • 26
  • Such dual factories often exist because, although the objects are very distinct, the configuration to create them is significantly shared and it'd be problematic to split that aspect. Alternately I've seen cases where the two output types are independent interfaces, but in this factory a single object can actually satisfy both roles. – edA-qa mort-ora-y Jul 15 '11 at 20:06
1

As a general rule, if it involves auto_ptr, it's not idiomatic. In general, the structure is not idiomatic too- normally, you'd make one function for each, return by value and throw an exception if they fail, and if you need to share variables, make it an object.

Puppy
  • 144,682
  • 38
  • 256
  • 465
  • Yes, if I could elegantly split the method into two, I would have done that. I am not masochistic, after all ;-) – Dilum Ranatunga Jul 15 '11 at 18:22
  • "As a general rule, if it involves auto_ptr, it's not idiomatic" -- what is the idiomatic alternative to an `auto_ptr`? – Dilum Ranatunga Jul 15 '11 at 18:23
  • `auto_ptr` is one of the cleanest and most direct ways I can think of to indicate the caller is taking ownership. While `auto_ptr` is kind of broken, it's the best we have prior to `unique_ptr` in C++0x. – edA-qa mort-ora-y Jul 15 '11 at 20:11
  • There's a scoped_ptr in Boost that isn't hideously broken. – Puppy Jul 16 '11 at 11:45
1

Let's assume that a return value of false means "don't look at the output parameters".

Then what I would do is get rid of the bool return value, return a struct or pair that has the auto_pointers you want, and throw in the error condition.

Mark B
  • 95,107
  • 10
  • 109
  • 188
  • ... so to summarize, use a struct; the additional code is worth it because its more idiomatic? – Dilum Ranatunga Jul 15 '11 at 18:20
  • Yes, but as @Josh points out in his answer splitting it into two functions would be better. I would say that using out `auto_ptr`s is definitely not idiomatic. – Mark B Jul 15 '11 at 18:27
  • If you really need to have two outputs I find the struct solution is really sloppy in the current standard; I much prefer two references are output variables. Obviously a solution with two functions to create each object would be cleaner, but in some cases this is simply not possible, or not reasonable. – edA-qa mort-ora-y Jul 15 '11 at 20:16
1

Usually when you have auto_ptr parameters they are not references.

This is because when you pass something to a function that takes auto_ptr you are expecting that function to take ownership. If you are passing by reference it does not actually take the object (it may take the object).

Its a subtle point, but in the end you need to look at what your interface is trying to say to the user.

Also you seem to be using it as an out parameter.
Personally I have never seen this use case (but I can see it) just document what you are trying to do and more importantly why.

Martin York
  • 257,169
  • 86
  • 333
  • 562
  • It is absolutely an *out* parameter. I make sure they are named accordingly. In addition to comments, what more can I do to make the use clear? – Dilum Ranatunga Jul 15 '11 at 18:18
  • @Dilum Ranatunga: I think return values as the result of the function is probably a better idea than having out parameters. But if you must have then a smart pointer is the way to go. – Martin York Jul 15 '11 at 19:22
  • Taking parameters by reference is a very standard C++ way of having a function return multiple values. Nobody really likes doing this, but the alternatives are currently just as messy. Multiple named return values would certainly be nice. – edA-qa mort-ora-y Jul 15 '11 at 20:14
  • @edA-qa mort-ora-y: Actually I have to disagree. This is normal in C but not C++ (or good C++). If you want to return multiple values you use boost::tie() and boost::tuple(). But that is not to say it does not happen. – Martin York Jul 15 '11 at 23:34
1

You don't have to make up your own struct to return two values - you can use std::pair. In that case there isn't much syntactic overhead in returning the two values. This solution does have the problem that ".first" and ".second" aren't very descriptive names, but if the types involved and the name of the function make the intent clear enough then that's not necessarily a problem.

If you are using C++0x you could use unique_ptr insted of auto_ptr and the caller can use auto instead of having to type the longer std::pair<std::unique_ptr<A>, std::unique_ptr<B>>. If you are not using C++0x you might consider using a typedef for that instead.

If you return the two values then you won't have space for the bool. You could use a C++0x tuple to return all three values. You could also indicate error by throwing an exception or by returning null pointers. I would prefer an exception assuming that the error is rare/exceptional.

As other answers have pointed out, it is often preferable to have two separate functions that each return a single object. If you can't do that because the initialization of the two objects is inextricably linked then you could make a class that encapsulates the initialization. You could pass the necessary information to make the two objects to the constructor (requires exception to signal errors) and then have two methods on that class that yield one object each.

Bjarke H. Roune
  • 3,667
  • 2
  • 22
  • 26
  • yes, for two values, `std::pair` works. When there's more, in my non C++0x land I have to resort to boost tuples or writing structs. – Dilum Ranatunga Jul 16 '11 at 00:34