1

I have a class, Table, which contains a member height. The value of height should be either an int or point to an object which has additional data.

The purpose here is that the user can either input a straight value, or choose a predefined value (identified by its ID) which can be shared by multiple Table objects.

This is a way I think I could achieve this functionality.

1) Create a base class Height.

2) Have two subclasses StraightHeight and PredefinedHeight

StraightHeight would simply contain an integer with the height (with getter and setter), and PredefinedHeight would contain the additional data:

class PredefinedHeight : public Height
{ 

    public: 

    void setHeight(int height);
    void setId(int id);
    void setName(std::string name);

    int getHeight();
    int getId();
    std::string getName();

   private:

   int height;
   int id;
   std::string name;

}; 

Inside Table, the height member would be of type Height.

I think this would work in practice, however I have another issue. When it comes to displaying this data, if height is a straight value (of type StraightHeight), it should just display the int height. If it is a PredefinedHeight, it should display the id and name as well. I could have a virtual method std::string getData() in both classes which returns the necessary data but it feels messy relying on the classes to format the string, etc.

Alternatively I could use a dynamic cast, but worry that it may be bad practise in this case.

Or for simplicity sake, not bother with inheritance and just have one class for Height. If it's a straight value, just leave the additional members empty. Does anyone have a better suggestion?

19172281
  • 237
  • 2
  • 10
  • 2
    1) Inheritance does not work this way in C++. 2) Not sure what that means. Your description "The value of height should be either an int or point to a struct which has additional data" describes ***exactly*** C++17's `std::variant`. However, you probably haven't yet studied templates yet, and are not likely to know how to use it. With core C++, the cleanest way to do this would be, I say, make `Height` and abstract class, and create two subclasses. – Sam Varshavchik May 24 '20 at 23:52
  • What you are suggesting is what I meant in 2). Have a base class and 2 subclasses, 1 with just one member variable `height` and the other with the additional members. – 19172281 May 24 '20 at 23:59
  • "If [...] the reference changes, I don't need to update each object" That's where the problem is: it's false, unless your data is hidden from the user. Your app should be responsive to data changes, internally implementing something that serves as a model for the data, and another thing that views the data. The view certainly will need to know about any referenced data that got changed, and it will in effect have to propagate those changes in order to update the view. So, the references will need to be observable: the view observes the reference and reacts to change of the referenced data. – Kuba hasn't forgotten Monica May 25 '20 at 00:13
  • @ReinstateMonica, not sure I get you. Suppose I have multiple objects with a `std::string` pointer pointing to the same string. If I change the value of the string, surely this would reflect in the objects when I dereference the pointer? – 19172281 May 25 '20 at 00:23
  • Yeah, But how will you know that you need to read the string and e.g. update the user display? You won't. So this data model you propose is fine if the view of the data is static and generated only once. Otherwise it's not nearly enough, and you'd need to analyze the interaction of the change sources with the views of the data. But what you need to do first is to provide some justification for such references: they make sense for strings, but they may well make things worse when accessing integers. – Kuba hasn't forgotten Monica May 25 '20 at 00:51
  • You have inheritance without virtual functions. This most probably means you need a refresher on how object oriented software works and how inheritance implements object orientation paradigm. – n. m. could be an AI May 25 '20 at 19:16
  • @n.'pronouns'm., I don't see why the methods I've shown in `PredefinedHeight` should be virtual. Objects of that type are created and populated outside the `Table` class as `PredefinedHeight* h = new PredefinedHeight()` and stored in a vector. I did suggest introducing a virtual method for when the data needs to be accessed inside `Table`, and `height` is of type `Height`. In which case `height` could be a pointer to either `StraightHeight` or `PredefinedHeight`. – 19172281 May 25 '20 at 19:29
  • If you don't have any virtual functions, you are abusing/misusing inheritance. Either you need virtual functions, or you don't need inheritance. It's a simple and reliable rule of thumb. Works about 100% of the time. "height could be a pointer to either StraightHeight or PredefinedHeight" Now how would you ever know what it points to if you don't have virtual functions? – n. m. could be an AI May 25 '20 at 20:20
  • @n.'pronouns'm., Well I would have a virtual function `getData()` for returning a string with the necessary data to be displayed. – 19172281 May 25 '20 at 20:30
  • Are you suggesting that all those methods should be virtual? – 19172281 May 25 '20 at 20:31
  • "all those methods should be virtual" No, not at all. If displaying the height is the only behaviour users of Height need, then getData is the only function that should be virtual. If plain Height doesn't have a name, then it should not have setName either. BTW relying on getters and setters is a sign of bad design, but that's a different story. – n. m. could be an AI May 25 '20 at 21:12
  • @n.'pronouns'm., Gotcha. Do you have any thoughts on the several approaches I specified in my question? – 19172281 May 25 '20 at 22:44
  • What is the purpose of the `name` member? What has "name" got to do with height?? – Kuba hasn't forgotten Monica May 26 '20 at 04:47
  • @ReinstateMonica, name is a description of the pre-defined value. It does make sense in the context of the application. – 19172281 May 26 '20 at 11:37
  • I would try to devise a system whereby a description of a value is separate from the value itself. I would also make the description class generic, independent of the kind of the value (height, weight or whatever). Perhaps templated on the type of the value. I would also make it abstract, with concrete descendants representing a trivial description (in place of your naked Height), detailed description (your PredefinedHeight) etc. I would **not** use std::variant or any other kind of union here. – n. m. could be an AI May 26 '20 at 11:45
  • @n.'pronouns'm. Thanks. If you have the time, would you consider elaborating in an answer. I like examples :) – 19172281 May 26 '20 at 12:11

2 Answers2

3

Note: This is an updated, rewritten answer. The prior version is available in the edit history.

Let's look at it from data modeling perspective: Table has a property height. That property can either be owned by the Table, or can come from another source. The simplest way one could think of would be to use a pointer to a const object. The pointer should be to a const, since the height may be a preset, and those shouldn't be changeable via the Table object.

class Table {
  int m_myHeight;
  const int *m_height = &m_myHeight;
public:
  int height() const { return *m_height; }
  void setHeight(int newHeight) {
    m_height = &m_myHeight;
    m_myHeight = newHeight;
  }
  void setPresetHeight(const int &preset)
  {
    m_height = &preset;
    /* this line is optional */ m_myHeight = *m_height;
  }
};

class PresetHeights {
  std::vector<int> m_data;
public:
  const int &getPreset(int index);
};

This will work just fine, but you may wish to have some additional properties assigned to the preset - properties that the "embedded" height of the Table object doesn't have. For example, the preset can have a name, etc.

This can be done by holding a reference to either "just" a height, or to a height preset. Since the identifier of a height preset is used to index the identifier in a presets table, it probably makes sense to make the id value private, and only accessible via the presets table. This gives the power over ids to the presets table, and gives some freedom in how the table is implemented.

The example that follows is written in C++17, and can be tried out on godbolt.

#include <string>
#include <variant>

struct Height {
  int value;
  Height(int value) : value(value) {}
  operator int() const { return value; }
};

struct HeightPreset : Height {
  using Id = int;
private:
  Id id; // the unique identifier of this preset
  friend class HeightPresets;
  friend int main(); // test harness
public:
  std::string name; // name of this preset
  template <typename Name>
  HeightPreset(Id id, int value, Name &&name) : 
    Height(value), id(id), name(std::forward<Name>(name)) {}
};

The Table uses std::variant to hold either no height value (std::monostate), or a Height, or a HeightPreset:

#include <functional>

class Table {
  using preset_t = std::reference_wrapper<const HeightPreset>;
  std::variant<std::monostate, Height, preset_t> m_height;
public:
  std::optional<Height> height() const {
    if (auto *customHeight = std::get_if<Height>(&m_height))
      return *customHeight;
    else if (auto *presetHeight = std::get_if<preset_t>(&m_height))
      return std::get<preset_t>(m_height).get();
    else
      return {};
  }
  void setHeight(Height newHeight)
  { m_height = newHeight; }
  void setHeightPreset(const HeightPreset &preset) 
  { m_height = std::cref(preset); }
  bool hasPresetHeight() const { return m_height.index() == 2; }
  const HeightPreset &presetHeight() const
  { return std::get<preset_t>(m_height).get(); }
};

Presets Iterable By Value of Type HeightPreset

The presets are a map from HeightPreset::Id to HeightPreset. But first, we need an iterator adapter to let us iterate the preset values - hiding the implementation detail that we use a map whose iterated values are std::pair, not HeightPreset.

#include <map>

template <class K, class V, class C, class A>
class map_cvalue_iterator
{
  typename std::map<K, V, C, A>::const_iterator it;
public:
  map_cvalue_iterator(typename std::map<K,V>::const_iterator it) : it(it) {}
  map_cvalue_iterator(const map_cvalue_iterator &o) : it(o.it) {}
  auto &operator=(const map_cvalue_iterator &o) { it = o.it; return *this; }
  auto operator++(int) { auto val = *this; ++it; return val; } 
  auto &operator++() { ++it; return *this; }
  auto operator--(int) { auto val = *this; --it; return val; } 
  auto &operator--() { --it; return *this; }
  const V& operator*() const { return it->second; }
  const V* operator->() const { return it->second; }
  bool operator==(map_cvalue_iterator o) const { return it == o.it; }
  bool operator!=(map_cvalue_iterator o) const { return it != o.it; }
};

template <class M>
using map_cvalue_iterator_type 
  = map_cvalue_iterator<typename M::key_type, typename M::mapped_type,
                        typename M::key_compare, typename M::allocator_type>;

The presets are a thin wrapper around std::map:

class HeightPresets {
public:
  using Id = HeightPreset::Id;
  HeightPresets(std::initializer_list<HeightPreset> presets)
  {
    for (auto &preset : presets)
      m_presets.insert({preset.id, preset});
  }
  auto &get(Id id) const { return m_presets.at(id); }
  Id getIdFor(const HeightPreset &preset) const 
  { return preset.id; }

  auto begin() const { return  map_cvalue_iterator_type<map_t>(m_presets.cbegin()); }
  auto end() const { return map_cvalue_iterator_type<map_t>(m_presets.cend()); }
private:
  using map_t = std::map<Id, HeightPreset>;
  map_t m_presets;
};

The simple test harness that demonstrates the use of those types:

#include <cassert>

int main() {
  const HeightPresets presets{
    {1, 5, "A Fiver"},
    {2, 10, "A Tenner"}
  };
  Table aTable;

  assert(!aTable.height());
  // The table has no height by default

  aTable.setHeight(10);
  assert(!aTable.hasPresetHeight());
  // The height was not a preset
  assert(aTable.height() == 10);
  // The height was retained
  
  for (auto &preset : presets)
  {
    aTable.setHeightPreset(preset);
    assert(aTable.hasPresetHeight());
    // The height was preset
    assert(aTable.height() == preset);
    // The height has the expected preset's value
    assert(presets.getIdFor(aTable.presetHeight()) == preset.id);
    // The height has the expected preset's identifier
    assert(aTable.presetHeight().name == preset.name);
  }
}

Presets Iterable by std::pair<HeightPreset::Id, HeightPreset>

If you wish not to use the iterator adapter, it can be removed. See below, and also try it out on godbolt.

#include <map>

class HeightPresets {
public:
  using Id = HeightPreset::Id;
  HeightPresets(std::initializer_list<HeightPreset> presets)
  {
    for (auto &preset : presets)
      m_presets.insert({preset.id, preset});
  }
  auto &get(Id id) const { return m_presets.at(id); }
  Id getIdFor(const HeightPreset &preset) const 
  { return preset.id; }

  auto begin() const { return  m_presets.cbegin(); }
  auto end() const { return m_presets.cend(); }
private:
  using map_t = std::map<Id, HeightPreset>;
  map_t m_presets;
};

And the test harness:

#include <cassert>

int main() {
  const HeightPresets presets{
    {1, 5, "A Fiver"},
    {2, 10, "A Tenner"}
  };
  Table aTable;

  assert(!aTable.height());
  // The table has no height by default

  aTable.setHeight(10);
  assert(!aTable.hasPresetHeight());
  // The height was not a preset
  assert(aTable.height() == 10);
  // The height was retained
  
  for (auto &presetPair : presets)
  {
    auto &preset = presetPair.second;
    aTable.setHeightPreset(preset);
    assert(aTable.hasPresetHeight());
    // The height was preset
    assert(aTable.height() == preset);
    // The height has the expected preset's value
    assert(presets.getIdFor(aTable.presetHeight()) == preset.id);
    // The height has the expected preset's identifier
    assert(aTable.presetHeight().name == preset.name);
  }
}
Community
  • 1
  • 1
Kuba hasn't forgotten Monica
  • 95,931
  • 16
  • 151
  • 313
  • Thank you, but the level of code shown is far too advanced for me at this time. I was hoping to achieve this without templates, variants, etc. Could your first example be adapted to allow for the additional data without templates, variants, etc? – 19172281 May 26 '20 at 12:18
  • Actually, reading up on Variants, I think I'm beginning to understand how they work. It's the `map_cvalue_iterator` section that I'm really struggling with. – 19172281 May 26 '20 at 13:45
  • When you iterate a `std::map`, it's giving you values that are `std::pair`. Usually you'd want to iterate just over `MappedValue`, and that iterator adapter transforms a map iterator that gives out pairs to one that gives out just the second element of the pair. It's only for convenience and not otherwise necessary. In the `for (auto &preset : presets)` loop body, you would need to replace `preset` with `preset.second`, and change the `begin()` and `end()` in `HeightPresets` to return map iterators directly e.g. `auto begin() const {return m_presets.begin();}` – Kuba hasn't forgotten Monica May 26 '20 at 16:52
  • Cheers, I'll go with that. Out of interest, how would you have approached this problem before Variant, using "basic" C++? I can think of one very crude way. In `Table` have an `int height` and `HeightPreset* preset` If `preset` is null, then use `height`, else use `preset`. I see no reason why it wouldn't work, although it might not be the neatest way. – 19172281 May 26 '20 at 20:41
  • 1
    Of course it'd work, if you want something minimalistic. But before C++11 you had `boost::variant`, and any sizable project would use either that, or a similar library. It all depends on how complex the rest of the code is. Usually projects that use "basic" C++ become a mess of boring copypasta spaghetti. `boost` and its brethren were created because they solve common problems well. I'm working on porting a legacy project to modern C++ and 50% of the code can be easily removed, the other 15% with more work. That's what it gets to when you stick with "basic" C++. Pointers all over the place :/ – Kuba hasn't forgotten Monica May 26 '20 at 21:20
  • Thank you for all your help. I'm finally learning to not be so afraid of these libraries. :) – 19172281 May 26 '20 at 21:27
1

The value of height should be either an int or point to a struct which has additional data

This is called a sum type a.k.a. a tagged union.

C++ has a standard template for that: std::variant from the standard <variant> header. You probably want to use some smart pointer inside.

If you cannot use that header, re-implement it using some union inside your class (with another member discriminating that union, inspired by this), but don't forget to follow the rule of five.

I recommend reading more about C++. First, Programming -- Principles and Practice Using C++ and later the C++11 standard n3337.

Basile Starynkevitch
  • 223,805
  • 18
  • 296
  • 547