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 
#include 
#include "pair.h"

typedef std::valarray ArrayInt;
typedef Pair 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 & 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 > holdings.first()[i];
        cout > holdings.second()[i];
    }
}

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

… along with the author's Pair<> temp

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 ArrayInt;
typedef Pair PairArray;

I would have done this:

typedef Pair         WineInfo;
typedef std::vector  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& 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
Wine::Wine(const char *l, const int (yrs&)[y], const int (bot&)[y])
    : mLabel(l), yearsHeld(y)

Always hat getters. They break encapsulation.

std::string & 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& out = std::cout) const

Always prefer \n over std::endl

cout << "Wine: " << mLabel << 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 < 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& first() const { return a; }
  T2 const& second() const { return b; }

StackExchange Code Review Q#114209, answer score: 5