Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a class that delegates some responsibilities to another helper class which is abstract. When instantiating the class the user needs to provide a pointer to a concrete instance of the abstract helper class. I'm wondering who should be responsible for deleting the helper class instance, the user, or the class itself?

It seems to me that it is better for the class to delete the helper since it ensures that the lifetime of the helper class instance is the same as the lifetime of the class thats using it.

P.S. I know shared pointers can help but I'd rather avoid them here.

Specifically which is better, this:

classifier.h

class Classifier {
 public:
  Classifier(const Trainer* trainer);
  Train(const RecordList& training_records);
  ~Classifier();
 private:
  const Trainer* trainer_;
  Vector parameters_;
}

classifier.cpp

Classifier::Classifier(const Trainer* trainer) 
    : trainer_(trainer);

Classifier::Train(const RecordList& training_records) {
  parameters = trainer_->UpdateParameters(training_records);
}

Classifier::~Classifier() {
  delete trainer_;
}

main.cpp

int main() {
  Classifier classifier(new SgdTrainer());
}

Or is this better?

classifier.h

class Classifier {
 public:
  Classifier(const Trainer* trainer);
  Train(const RecordList& training_records);
 private:
  const Trainer* trainer_;
  Vector parameters_;
}

classifier.cpp

Classifier::Classifier(const Trainer* trainer) 
    : trainer_(trainer);

Classifier::Train(const RecordList& training_records) {
  parameters = trainer_->UpdateParameters(training_records);
}

main.cpp

int main() {
  SgdTrainer trainer();
  Classifier classifier(&trainer);
}
share|improve this question
    
Post rolled back as it invalidated answers. –  Jamal Jun 27 at 4:06

1 Answer 1

up vote 5 down vote accepted

It depends. (By the way, as things are your Classifier::Train function is ill-formed due to missing a return type; I'll assume void.)

If you expect the value to need dynamic storage duration, use the first method, but use a smart pointer (std::unique_ptr, boost::scoped_ptr or similar). That may remove the need for an explicitly-defined destructor; if not, remember the rule of three.

If you do not expect the value to need dynamic storage duration, use the second form. I recommend passing trainer by const reference, but it really doesn't matter all that much.

If you can't decide, use the latter unless you have a compelling reason to use the former.

share|improve this answer
    
Thank you. Trainer itself is an abstract class (it has pure virtual methods). Can I still pass it by const reference? –  padawan Nov 12 '12 at 3:42
    
@padawan: Yes, why not? –  Anton Golov Nov 12 '12 at 5:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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