Value store ("optional" type)

Question

Create a class template named ValueStore that has a member variable, to store a value and a member variable, hasValue that holds a true value if there is data in the value member variable and false otherwise. Use the class to store three values of different types.

#include <iostream>
using namespace std;

template <typename T>
class ValueStore {
private:
  T value;

public:
  bool hasValue = false;

  ValueStore(T val) {
    value = val;
    hasValue = true;
  }

  ValueStore() {
    value = {};
  }

  T getValue() {
    return value;
  }

};

int main ()
{
  ValueStore<string> v1("hello, world");
  if (v1.hasValue) {
    cout << "The value stored is: " << v1.getValue() << endl;
  }
  else {
    cout << "v1 has no value." << endl;
  }
  ValueStore<int>v2(1000);
  if (v2.hasValue) {
    cout << "The value stored is: " << v2.getValue() << endl;
  }
  else {
    cout << "v2 has no value." << endl;
  }
  ValueStore<double> v3;
  if (v3.hasValue) {
    cout << "The value stored is: " << v2.getValue() << endl;
  }
  else {
    cout << "v3 has no value." << endl;
  }
  return 0;
}

Answer 1

About these assignments

As someone who’s been teaching C++ for 15 holy shit, almost 20 years now, I have to say that these assignments are pretty terrible… especially the “vector” one. (To be clear, I mean the assignments are terrible… not the attempts. The attempts aren’t bad at all.)

I’ve always found the logic of “let’s teach by reimplementing what’s in the standard library” to beginners to be a little idiotic. As far as teaching useful skills goes:

1. it just wastes the learner’s time learning how to do something they (almost certainly) will never need to do again; and
2. in the vast majority of cases, it requires such terrifyingly advanced C++ knowledge to properly replicate a standard library facility, that in order to make it doable at all you have to dumb it down to the point where it’s almost broken… which only gives the learner a false sense of confidence thinking they understand how to do the thing when they really don’t.

In this case, it looks like the goal is to reimplement std::optional and std::vector… but only half-assed, broken, functionally-useless variants of both. I see beginners attempting these things all the time, but what they don’t seem to understand is that those are INCREDIBLY difficult tasks, requiring knowledge of C++ that is WAY beyond what any beginner should be expected to have. Even some expert level C++ programmers don’t have the knowledge necessary to do these things. In particular, reimplementing std::vector properly is among the hardest things you can possibly do in C++. Just because it’s easy to use doesn’t mean it’s easy to make.

I wish it would become more commonly understood that reimplementing standard library facilities is a pants-crappingly stupid way of teaching C++. Reimplementing standard library facilities is an excellent project… for highly advanced C++ programmers. But beginners? No. Don’t teach beginners to reimplement the standard library… teach them to fricken’ use it!

Anywho, the problem here isn’t the learner—it’s the teacher who gave these crappy assignments—so I won’t rant any more about that. Instead I’ll just focus on the code.

I’m only going to review the first program here. The second program, I’ll review in a second reply. It looks like the recommendation is to split the two programs into two separate questions; so you should probably do that.

Code review

using namespace std;

Never, ever do this at file scope. This is a terrible practice. In fact, you should probably never do this ever, not even at function scope.

Among the many reasons this is bad: Today your code compiles because there is nothing called ValueStore in the standard library. But if some future version of the standard adds something with that name… boom, your code is now broken. And that’s if you’re lucky. What might also happen is that something new gets added to the standard library that silently changes the behaviour of your code. That’s a nightmare scenario.

Just use std:: where necessary.

  ValueStore(T val) {
    value = val;
    hasValue = true;
  }

The first issue with this constructor is that it’s not explicit. That makes it dangerous, because it will silently convert anything that is implicitly convertible to a T. At the very least, that is likely to be inefficient.

In general, any constructor that takes a single argument should be explicit. (Yes, that should actually be the default, but unfortunately, the default was set to implicit way back when C++ was first standardized, and now it’s too late to change.)

The second issue with this constructor is that it doesn’t use member initializer lists. What’s actually happening above is this:

ValueStore(T val)
:
    value{},        // value is default-constructed
    hasValue{false} // hasValue is initialized to false (because of the member initializer where it is declared)
{
    value = val;        // value is copy-assigned using val
    hasValue = true;    // hasValue is move-assigned using true
}

As you can see, value is set up twice. First it is default constructed, then it is assigned (with the actual contents of val).

What you should do, as a start, is this:

explicit ValueStore(T val)
:
    value{val},     // value is copy-constructed using val
    hasValue{true}  // hasValue is initialized to true
{
    // nothing to do here
}

Here you can see value is only set up once: it is initialized right from the start with val, rather than being initialized “empty” (technically default-constructed) then assigned.

But there’s one more improvement you could make. To understand it, consider the following code:

ValueStore<std::string> v1("hello, world");

What’s happening here is:

1. Before the ValueStore constructor is called:
   1. A temporary std::string is constructed with the contents “hello, world”.
   2. The temporary string is moved to val. (It is moved, not copied, because it is a temporary. As a practical matter, this whole step is usually elided as an optimization, meaning the temporary string is constructed directly into val.)
2. The ValueStore constructor is called:
   1. The member initializer list runs:
      1. val is copied into value.
      2. hasValue is set to true.
   2. The constructor body is called:
      1. Nothing happens here, because it’s empty.

So a temporary string is constructed, and then moved into val, and then copied into value. The copy is the problem there, because copying a string is expensive. Copying is generally expensive compared to moving, so you want to move whenever possible.

This is an easy fix; just move val into value:

explicit ValueStore(T val)
:
    value{std::move(val)},  // value is move-constructed using val
    hasValue{true}          // hasValue is initialized to true
{
    // nothing to do here
}

That’s all you need to do. Now a temporary string is constructed, then moved into val, then moved into value. In all likelihood, those moves can be elided. But even if not, moves are (usually) fast, especially for strings, and are usually no-fail.

There are a few other improvements you could make to this constructor:

constexpr explicit ValueStore(T val)
    noexcept(std::is_nothrow_move_constructible_v<T>)
:
    value{std::move(val)},
    hasValue{true}
{}

Making the constructor constexpr means you can construct ValueStore objects at compile-time, which can be enormously useful.

If move-constructing value cannot possibly fail, then the whole constructor cannot possibly fail. Adding the noexcept tells the compiler that, allowing it to make additional optimizations. (If move-constructing value can fail, then std::is_nothrow_move_constructible_v<T> will be false.)

  ValueStore() {
    value = {};
  }

This has the same problem as the previous constructor: you’re setting the value of value twice. What’s really happening is this:

ValueStore()
:
    value{},        // value is default-constructed
    hasValue{false} // hasValue is initialized to false
{
    value = {}; // a temporary T object is default-constructed,
                // then move-assigned into value
}

As you can see, that entire value = {}; is just a complete waste of time. All it does is take the already default-constructed value, default-constructs a whole new object, and then moves that into value. If you’d just left value alone, it would already be default-constructed:

ValueStore()
/*
:
    value{},        // value is default-constructed
    hasValue{false} // hasValue is initialized to false
*/
{
    // nothing to do here
}

But wait, there’s more!

Writing the default constructor like ValueStore() {} is actually a bad idea, because if it were possible to trivially-construct ValueStore objects… you just broke that functionality. Trivial construction means the compiler doesn’t actually have to do anything to construct a ValueStore, which means a lot of potential optimizations. It is better to leave the default constructor alone unless you actually need it to do something specific… which you don’t in this case.

So what you should write is:

ValueStore() = default;

And of course, you can add the extra decorations mentioned for the previous constructor to get even more benefits:

constexpr ValueStore() noexcept(std::is_nothrow_default_constructible_v<T>) = default;

(is_nothrow_default_constructible_v<T> is true only if default-constructing a T cannot fail.)

  T getValue() {
    return value;
  }

Since this function doesn’t change the state of the object, it should be const.

You could also add constexpr and maybe noexcept(std::is_nothrow_copy_constructible_v<T> and std::is_nothrow_copy_assignable_v<T>).

Now, this is really getting into complicated territory… but as the default choice for a new API, doing what you’ve done—returning a copy of value—is probably the right choice. However… this is not optimal, because if you don’t actually want a copy of value—if you just want to see what that value is (for example, just to print it like you’re doing in main()—you’re wasting resources by creating the copy.

What you really want to do is return a reference to value… but you have to be careful what kind of reference you return. If the ValueStore object is const, you don’t want to return a non-const reference to value… that would (theoretically) allow users to change the value of your const object, which is absurd. So you need two functions: one for const objects, and one for non-const objects:

constexpr T const& getValue() const noexcept { return value; }
constexpr T&       getValue()       noexcept { return value; }

Oh, but we’re not done yet, not by a long shot. Because there’s still the problem of temporaries. If you have a temporary ValueStore object, and you call getValue() on it, you’ll get a reference to value… but that reference will be dangling because the temporary is destroyed.

So you need to return rvalue references to value when the ValueStore is an rvalue (temporary, basically), and lvalue references when the ValueStore is an lvalue… and you need to take const into account. So what you really need is:

constexpr T const&  getValue() const&  noexcept { return value; }
constexpr T&        getValue() &       noexcept { return value; }
constexpr T const&& getValue() const&& noexcept { return std::move(value); }
constexpr T&&       getValue() &&      noexcept { return std::move(value); }

(Yes, you have to basically write the same function 2–4 times. That’s an unfortunate situation that some people are currently working to fix.)

((Rant mode again: See, complicated crap like this is why reimplementing std::optional is such a terrible project for beginners. And we haven’t even scratched the surface of just how complicated this really gets! You see, your class actually lies: it says hasValue is false if you do ValueStore<double> v3; But v3 does have a value: it is holding a double whose value is 0.0. Zero is a perfectly valid value for a double. Similarly, if you do ValueStore<std::string> v;, v.hasValue will say false… but v does have a value… it is holding a string whose current contents are just “”, but an empty string is still a string.))

That’s pretty much it for the ValueStore class. The only other comments I have are for the main() function.

First: Don’t use std::endl. Just don’t. Ever. Pretty much every use of std::endl I’ve ever seen is wrong… and yes, every single use in your program is wrong.

In particular, this is absurd:

    cout << "v1 has no value." << endl;

If you want a newline, just use a newline:

    std::cout << "v1 has no value.\n";

I know, every tutorial you’ve ever seen probably uses std::endl. Yes, I’m saying they’re all wrong. There is almost never a right time to use std::endl… and for those very, very rare situations where it has a purpose, you’re probably better off calling std::cout.flush() directly anyway, to make clear that’s what you want.

Finally: there is no need for return 0; in main(). main() automatically returns zero. (This is only true for main(); main() is special in a lot of ways.)

Summary

The problem I have with deciding whether this is a solid attempt at this problem or not is that the problem itself is stupid. Given a stupid problem, if someone’s solution isn’t great, is that because the solution is bad or because the problem is bad? It’s not easy to say.

I suppose this is a decent swing at the problem for a beginner. But really, this is not a beginner problem. Not if you take it seriously, anyway.

So long as you understand that hasValue is a lie, I suppose you can walk away from this having learned something. The truth is that your ValueStore always has a value. It can’t not have a value, so long as it has a T data member. Doing value = {} doesn’t set value to “not a value”; it sets it to the default value. Which is a value. There is literally no way to set value to “not a value”; that is literally impossible in C++.

You could cheat a little and do something like this:

template <typename T>
class ValueStore
{
private:
    std::optional<T> value;

public:
    bool hasValue = false;

    constexpr explicit ValueStore(T val)
    :
        value{std::move(val)},
        hasValue{true}
    {}

    constexpr ValueStore() noexcept = default;

    constexpr T const&  getValue() const&  noexcept { return value.value(); }
    constexpr T&        getValue() &       noexcept { return value.value(); }
    constexpr T const&& getValue() const&& noexcept { return std::move(value).value(); }
    constexpr T&&       getValue() &&      noexcept { return std::move(value).value(); }
};

Now ValueStore can have no value, because it is no longer holding a T, it’s holding a std::optional<T>… but now hasValue is redundant: it’s just duplicating value.has_value(). You could just do:

template <typename T>
class ValueStore
{
private:
    std::optional<T> value;

public:
    constexpr explicit ValueStore(T val) : value{std::move(val)} {}

    constexpr ValueStore() noexcept = default;

    constexpr T const&  getValue() const&  noexcept { return value.value(); }
    constexpr T&        getValue() &       noexcept { return value.value(); }
    constexpr T const&& getValue() const&& noexcept { return std::move(value).value(); }
    constexpr T&&       getValue() &&      noexcept { return std::move(value).value(); }

    constexpr bool hasValue() const noexcept { return value.has_value(); }
};

Anywho, back to your code: As I said, I think it’s a decent attempt at a very, very hard problem. In particular, your code is correct, for the vast majority of use cases… it’s just not efficient. Which is not a bad start! It’s always good to get it right first, and get it fast later, if possible.

Answer 2

Your ValueStore template class always has a value. If you don't call the one-argument constructor, it holds a default-constructed value.

That might work OK if the type T has a default. Just chalk it up to your teacher's language being imprecise, as we see with other phrases like "Use the class to store three values of different types." (Instantiate the template to make three classes.)

To actually do what it stated, you would need to use a pointer.

You don't need a separate flag, since you can check for the pointer being null.

 template <typename T>
 class ValueStore {
    ValueStore() = default;
    explicit ValueStore (T val) : p{make_unique<T>(std::move(val)} {}
    bool hasvalue() const { return p; }
    T getvalue() 
      {
      if (!p) throw // .... something
      return *p;
      }
 private:
    std::unique_ptr<T> p;
 };

I think the instructor meant that hasvalue is a member function that the user can call to determine whether there is a stored value. You have it as an implementation detail that is not accessible, and is useless since getValue always returns what's stored even if it's the default.

It makes more sense to be able to tell whether a value is stored and then the code can avoid fetching it if not present. But what happens if the caller fetches it anyway? Presumably it ought to throw an exception.

Note that the real implementation of std::optional will avoid the inefficiency of allocating heap memory by including a buffer as part of the class. But it still uses new to construct the object (using placement new syntax) when it exists and has no object constructed at all when empty.

Answer 3

Once thing that I feel particularly strongly about:

if (v3.hasValue) {
  cout << "The value stored is: " << v2.getValue() << endl;
}
else {
  cout << "v3 has no value." << endl;
}

The duplication of this code should be removed if at all possible. It's a maintenance nightmare, it makes life harder for the reader, and it's prone to errors (the example I copied does in fact have a cut/paste error, in that it prints the value of v2 rather than v3). Putting similar code into a ValueStore::Print() method is arguably the best way.