A classy wine collection - Construction vs. Initialization The following is for a chapter exercise in C++ Primer Plus, 6/e by Stephen Prata, for the first part of Chapter 14 which deals with code reuse through composition. The problem is to implement a Wine class (for modelling a wine collection) which contains a std::string (the wine name), an int and a Pair of std::valarrays to hold the vintages and bottles held of each. (Pair is a template class provided by the author.)

My question is whether I've got a good solution for the Wine constructors, or if I should be setting up my Pair in the initializer list (and what that would look like.)

Here's my header…

/**
* @file     winec.h
* @brief    Wine class with containment
*/

#ifndef WINEC_H_
#define WINEC_H_

#include &lt;string&gt;
#include &lt;valarray&gt;
#include "pair.h"

typedef std::valarray&lt;int&gt; ArrayInt;
typedef Pair&lt;ArrayInt, ArrayInt&gt; PairArray;

class Wine
{
private:
    std::string mLabel;
    PairArray holdings;
    int yearsHeld;

public:
    Wine();
    Wine(const char *l, int y);
    Wine(const char *l, int y, const int yrs[], const int bot[]);

    void getBottles();
    std::string &amp; label();
    int sum() const;
    void show() const;
};

#endif


… and my implementation…

/**
* @file     winec.cpp
* @brief    Wine class with containment
*/

#include "winec.h"

Wine::Wine()
    : mLabel("Unnamed"), yearsHeld(0)
{
    holdings.first() = ArrayInt(0);
    holdings.second() = ArrayInt(0);
};


Wine::Wine(const char *l, int y)
    : mLabel(l), yearsHeld(y)
{
    holdings.first() = ArrayInt(0, y);
    holdings.second() = ArrayInt(0, y);
}


Wine::Wine(const char *l, int y, const int yrs[], const int bot[])
    : mLabel(l), yearsHeld(y)
{
    holdings.first() = ArrayInt(y);
    holdings.second() = ArrayInt(y);

    for (int i = 0; i &lt; yearsHeld; i++)
    {
        holdings.first()[i] = yrs[i];
        holdings.second()[i] = bot[i];
    }
}


std::string &amp; Wine::label()
{
    return mLabel;
}


void Wine::show() const
{
    using std::cout;
    using std::endl;

    cout &lt;&lt; "Wine: " &lt;&lt; mLabel &lt;&lt; endl;
    cout &lt;&lt; "\tYear\tBottles\n";
    for (int i = 0; i &lt; yearsHeld; i++)
    {
        cout &lt;&lt; "\t" &lt;&lt; holdings.first()[i];
        cout &lt;&lt; "\t" &lt;&lt; holdings.second()[i];
        cout &lt;&lt; endl;
    }
}


void Wine::getBottles()
{
    using std::cin;
    using std::cout;

    cout &lt;&lt; "Enter holdings for " &lt;&lt; yearsHeld &lt;&lt; " years.\n";
    for (int i = 0; i &lt; yearsHeld; i++)
    {
        cout &lt;&lt; "Year: ";
        cin &gt;&gt; holdings.first()[i];
        cout &lt;&lt; "Bottles: ";
        cin &gt;&gt; holdings.second()[i];
    }
}


int Wine::sum() const
{
    int sum = 0;
    for (int i = 0; i &lt; yearsHeld; i++)
    {
        sum += holdings.second()[i];
    }
    return sum;
}


… along with the author's Pair&lt;&gt; template:


/**
* @file       pair.h
* @brief  Defining &amp; using a Pair template
*/

#ifndef PAIR_H_
#define PAIR_H_

#include &lt;iostream&gt;
#include &lt;string&gt;

template &lt;class T1, class T2&gt;
class Pair
{
private:
  T1 a;
  T2 b;

public:
  T1 &amp; first();
  T2 &amp; second();
  T1 first() const { return a; }
  T2 second() const { return b; }
  Pair(const T1 &amp;aval, const T2 &amp;bval) : a(aval), b(bval) {}
  Pair() {}
};


template &lt;class T1, class T2&gt;
T1 &amp; Pair&lt;T1, T2&gt;::first()
{
  return a;
}


template &lt;class T1, class T2&gt;
T2 &amp; Pair&lt;T1, T2&gt;::second()
{
  return b;
}

#endif
Code Review

Just wondering if there is an EC library inside windows.
Best to be descriptive and long winded here.

#ifndef WINEC_H_
#define WINEC_H_


Also you may want to put all your code inside a namespace.

In this scheme were you access the year and count you have to perform two array lookups. Not a big deal. But to me the logic seems the wrong way around.

typedef std::valarray&lt;int&gt; ArrayInt;
typedef Pair&lt;ArrayInt, ArrayInt&gt; PairArray;


I would have done this:

typedef Pair&lt;int, int&gt;         WineInfo;
typedef std::vector&lt;WineInfo&gt;  PairArray;


Now when you have the year (vintage) you have the count in the same object (better spacial locality).

Is this not simply the size of holdings?

    int yearsHeld;


I would not have a default constructor.
Is there a need to create an special "unnamed" object?

Sure you can pass by C-String    

Wine::Wine(const char *l, int y)


But I usually make this a std::string const&amp; then it can be used with strings and with C-Strings (the string is dynamically constructed in place). If you are worried about the copy you can always add a constructor that takes the parameter by r-value reference so it is moved into the object.

I don;t like passing y the number of years held. Then just creating a set of unitialized values. The year 0 is not really a valid value.

Wine::Wine(const char *l, int y)


What you could do is allow a list initializer. This takes a list of WineInfo that allows you to initialize the object.

Wine  myWine("Flore de Sole", {{2014,15},{2013,8},{2012,3},
                                {2011,1},{2010,1},{2009,1}}
            );


OK. So you try. But this is a very dangerous interface. There is no way to guarantee that the yrs array and the bot array are at least y size.

Wine::Wine(const char *l, int y, const int yrs[], const int bot[])
    : mLabel(l), yearsHeld(y)


You should pass references to objects of known size. You could pass a reference to an array of length y

template&lt;int y&gt;
Wine::Wine(const char *l, const int (yrs&amp;)[y], const int (bot&amp;)[y])
    : mLabel(l), yearsHeld(y)


Always hat getters. They break encapsulation.

std::string &amp; Wine::label()
{
    return mLabel;
}


Also note you are returning a non cost reference. So the caller can alter the label dynamically.

w.label() = "Plop";


Sure. Always good to have a print function. But rather than fix it with std::cout pass the stream as a parameter. The default can always be std::cout

void Wine::show() const


I would write as

void Wine::show(std::ostream&amp; out = std::cout) const


Always prefer \n over std::endl

    cout &lt;&lt; "Wine: " &lt;&lt; mLabel &lt;&lt; endl;


The difference is that std::endl will also flush the buffer. This makes using streams very in-effecient. You practically never need to flush the buffer manually.

Rather than using a manual loop. Prefer to use an algorithm

    for (int i = 0; i &lt; yearsHeld; i++)
    {
        sum += holdings.second()[i];
    }


Simpler to write:

    return std::accumulate(std::begin(holdings), std::end(holdings));


No need to copy if you just need a reference.

  T1 first() const { return a; }
  T2 second() const { return b; }


Just return by const reference. Then it can be used without having a copy made.

  T1 const&amp; first() const { return a; }
  T2 const&amp; second() const { return b; }