hzeller / txtempus

A DCF77, WWVB, JJY and MSF clock LF-band signal transmitter using the Raspberry Pi
GNU General Public License v3.0
429 stars 67 forks source link

Fix memory leak and minor refactoring #26

Closed pjueon closed 2 years ago

pjueon commented 2 years ago
pjueon commented 2 years ago

@hzeller

pjueon commented 2 years ago

Thanks for the feedback.

Removing the variable service looks good to me.

The factory function that returns the raw pointer still doesn't feel right to me though. You might reuse the the factory somewhere else in the future.

You can use std::make_unique to avoid repetetive std::unique_ptr<TimeSignalSource> in the factory:

std::unique_ptr<TimeSignalSource> CreateTimeSourceFromName(const char *n) {
  if (strcasecmp(n, "DCF77") == 0)
    return std::make_unique<DCF77TimeSignalSource>();
  if (strcasecmp(n, "WWVB") == 0)
    return std::make_unique<WWVBTimeSignalSource>();
  if (strcasecmp(n, "JJY40") == 0)
    return std::make_unique<JJY40TimeSignalSource>();
  if (strcasecmp(n, "JJY60") == 0)
    return std::make_unique<JJY60TimeSignalSource>();
  if (strcasecmp(n, "MSF") == 0)
    return std::make_unique<MSFTimeSignalSource>();
  return nullptr;
}

To use std::make_unique, C++14 or higher is required. You can also implement your own version of make_unique function if you don't want to change the C++ standard requirements:

template<class T>
std::unique_ptr<T> make_unque(){
  return std::unique_ptr<T>{new T()};
}
pjueon commented 2 years ago

Removed std::string service. Please let me know your opinion about make_unique. @hzeller

hzeller commented 2 years ago

I think switching to c++14 and using make_unique sounds good to me.

hzeller commented 2 years ago

Thanks, merged!