10
\$\begingroup\$

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

typedef std::valarray<int> ArrayInt;
typedef Pair<ArrayInt, ArrayInt> 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 < yearsHeld; i++)
    {
        holdings.first()[i] = yrs[i];
        holdings.second()[i] = bot[i];
    }
}


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


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

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


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

    cout << "Enter holdings for " << yearsHeld << " years.\n";
    for (int i = 0; i < yearsHeld; i++)
    {
        cout << "Year: ";
        cin >> holdings.first()[i];
        cout << "Bottles: ";
        cin >> 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<> template:

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

#ifndef PAIR_H_
#define PAIR_H_

#include <iostream>
#include <string>

template <class T1, class T2>
class Pair
{
private:
  T1 a;
  T2 b;

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


template <class T1, class T2>
T1 & Pair<T1, T2>::first()
{
  return a;
}


template <class T1, class T2>
T2 & Pair<T1, T2>::second()
{
  return b;
}

#endif
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

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

I would have done this:

typedef Pair<int, int>         WineInfo;
typedef std::vector<WineInfo>  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<int y>
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; }
\$\endgroup\$
3
  • \$\begingroup\$ I admit the Pair-of-valarrays had been bugging me as well; inverting it makes more sense and lets me use an initializer as well: holdings(VintageList(WineInfo(0,0), y)). My compiler doesn't like Wine::Wine(const char *l, const int (yrs&)[y], const int (bot&)[y]) and I can't get std::accumulate() to work with Pair<> but those are peripheral to the exercise, and I suppose I can always post them on SO. Thanks for your help. \$\endgroup\$ Dec 17, 2015 at 23:08
  • \$\begingroup\$ I forgot to ask: what did you mean by "EC library?" \$\endgroup\$ Dec 19, 2015 at 1:50
  • \$\begingroup\$ Half a joke: But all windows header files have WIN as the prefix,. If there was a windows library called EC. Then the header to this library would be WINEC \$\endgroup\$ Dec 19, 2015 at 4:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.