Using non-member non-friend functions instead of member functions: disadvantages?

user_7617755 0
Viewed 77

Scott Meyers has long advocated using non-member non-friend functions instead of member functions to improve encapsulation. I can see the advantages of this.

However, a disadvantage in my eyes is the following:

I have a custom metadata class for some custom image class, which contains a number of data members. There are a few formats in which the image can be saved and the metadata has to be converted into something those formats can take (ENVI, png, TIFF, ...). Now I followed Scott's advice and put these conversion functions into a seperate namespace. They essentially use the public interface to copy all members into something that fits the end metadata format, but they need to include all data members. Example:

// file Metadata.h
class Metadata
{
   // Getters
     std::string GetDescription() const;
     std::string GetTimeStamp() const;
     float       GetExposureTimeInMilliSeconds() const;
  // Setters
  // ...
   private:
     std::string m_description;
     std::string m_timeStamp;
     float       m_exposureTimeInMilliSeconds;

   // Added later with associated getters/setters:
   // std::string m_location;
   // std::string m_nameOfPersonWhoTookThePicture;
};

// File UtilityFunctions.h
namespace UtilityFunctions
{
    ENVIMetadata ConvertMetadataToENVIMetadata(const Metadata &i_metadata)\
    {
        ENVIMetadata envi;
        envi.AddMetadata<string>("Description", GetDescription());
        envi.AddMetadata<string>("Time stamp", GetTimeStamp());
        envi.AddMetadata<float>("Exposure time", GetExposureTimeInMilliSeconds());
    }
}

The problem I'm seeing is that when someone else would work on the project and that person would add another data member to the metadata, he/she needs to remember to add this data member to all the conversion functions. As they are located in a different header/cpp file, it is easy to forget this and we have a non-obvious bug that not all data members are saved in the metadata. If the function would be a public member, looking at the header file (while adding new data members) might remind that person to also add the members in there and then the necessity to be complete would only be in that file.

The takeaway is that using the public interface indeed guarantees (if the interface doesn't change) that functions based on it will keep working if something inside the class changes, but it does not guarantee completeness if additional features are added to the class, which need to be added to those functions as well.

Are there situations where one would advise not following this advice? Is there some paradigm for this specific situation to get the best of both worlds?

oop c++
2 Answers

I'm not sure I necessarily agree about the non-member vs member functions discussion since non-member certainly doesn't improve encapsulation. Regardless, I would recommend using structured bindings in C++17 to help with this problem.

// file Metadata.h
struct Metadata
{
     std::string m_description;
     std::string m_timeStamp;
     float       m_exposureTimeInMilliSeconds;

   // Added later:
   // std::string m_location;
   // std::string m_nameOfPersonWhoTookThePicture;
};

// File UtilityFunctions.h
namespace UtilityFunctions
{
    ENVIMetadata ConvertMetadataToENVIMetadata(const Metadata &metadata)
    {
        const auto& [description, timestamp, exposureTimeInMilliSeconds] =
            metadata;
        ENVIMetadata envi;
        envi.AddMetadata<string>("Description", description);
        envi.AddMetadata<string>("Time stamp", timeStamp);
        envi.AddMetadata<float>("Exposure time", exposureTimeInMilliSeconds);
    }
}

When the fields m_location and m_nameOfPersonWhoTookThePicture are later added, the structured bindings declaration will produce an error saying that you don't have enough identifiers provided.

user_29682840

You could provide a tuple-like view of Metadata, and have the conversion functions instantiate a std::index_sequence to populate the result;

// file Metadata.h
class Metadata
{
   // Getters
     std::string GetDescription() const;
     std::string GetTimeStamp() const;
     float       GetExposureTimeInMilliSeconds() const;

     template<size_t I> static const char * name();

  // Setters
  // ...
   private:
     std::string m_description;
     std::string m_timeStamp;
     float       m_exposureTimeInMilliSeconds;

   // Added later with associated getters/setters:
   // std::string m_location;
   // std::string m_nameOfPersonWhoTookThePicture;
};

namespace std
{
    template<> class tuple_size<Metadata> : public std::integral_constant<std::size_t, 3> {}; // later 5

    template<> class tuple_element<0, Metadata>{ using type = std::string; };
    template<> class tuple_element<1, Metadata>{ using type = std::string; };
    template<> class tuple_element<2, Metadata>{ using type = float; };
    /* Later add
    template<> class tuple_element<3, Metadata>{ using type = std::string; };
    template<> class tuple_element<4, Metadata>{ using type = std::string; };
    */
}

template<size_t I> std::tuple_element_t<I, Metadata> get(const Metadata & meta);
template<> std::string get<0>(const Metadata & meta) { return meta.GetDescription(); }
template<> std::string get<1>(const Metadata & meta) { return meta.GetTimeStamp(); }
template<> float get<2>(const Metadata & meta) { return meta.GetExposureTimeInMilliSeconds(); }
/* Later add
template<> std::string get<3>(const Metadata & meta) { return meta.GetLocation(); }
template<> std::string get<4>(const Metadata & meta) { return meta.GetPhotographerName(); }
*/

template<> const char * Metadata::name<0>() { return "Description"; }
template<> const char * Metadata::name<1>() { return "Time Stamp"; }
template<> const char * Metadata::name<2>() { return "Exposure Time"; }
/* Later add
template<> const char * Metadata::name<3>() { return "Location"; }
template<> const char * Metadata::name<2>() { return "PhotographerName"; }
*/

The conversion function then doesn't change when you add members

// File UtilityFunctions.h
namespace UtilityFunctions
{
    namespace detail
    {
        template<size_t... Is>
        ENVIMetadata ConvertMetadataToENVIMetadata(const Metadata &i_metadata, std::index_sequence<Is...>)
        {
            ENVIMetadata envi;
            envi.AddMetadata<std::tuple_element_t<Is, Metadata>>(Metadata::name<Is>(), get<Is>(i_metadata))...;
            return envi;
        }
    }

    ENVIMetadata ConvertMetadataToENVIMetadata(const Metadata &i_metadata)\
    {
        return detail::ConvertMetadataToENVIMetadata(i_metadata, std::make_index_sequence<std::tuple_size_v<Metadata>>{})
    }
}
user_26108100
Related