Need advice with a simple container class with templates C++ (code layout and readability)

Question

First of all, thanks for looking at this code. I'm a novice coder trying to learn C++ on my own so any help is much appreciated.

I've been working on a simple program to output values to the console as a learning project, and I stumbled across an article advising against using 2D containers, suggesting to simulate them instead with a 1D vector/container. I immediately proceeded to try to create a ridiculous class that converted input X and Y values into the correct position in the class's vector member. This class had a lot of redundant code, and eventually I realized that I was losing the value of the simplicity of the container having only one dimension by requiring two dimensions to input into the container. Instead I decided to move that math to other functions and just take a single input into the vector inside the class.

The following code is an attempt at a SimpleContainer class. I realize that I am reinventing the wheel here, but this is just a project to aid the learning process for me. I'm trying to learn C++ best practices, and things like good code layout and readability. I know that this class is missing some important optimization and features, and I think I'm doing unnecessary copying, but it would be very helpful for me to have a breakdown of what I'm doing right and wrong. This is also my first experience with using templates, but thankfully the code compiles, so I think I'm doing it right.

Anyway, I really hope that the answers here are helpful to others, and I appreciate the time of anyone that posts!

#include <iostream>    //Needed for std::cout and std::endl
#include <vector>      //Needed for std::vector and others

//These values are arbitrary and are only used when computing the
//screen size for things like memory allocation for the Container
#define CONSOLEWIDTH  80;
#define CONSOLEHEIGHT 25;

//The purpose of this class is to have an easy way to input variables and objects into a container,
//sometimes a large number of them at a time, and then retrieve and print them easily as well.
//Templating the class allows the input and printing of ints and chars very easily
//You could input objects, but some of the functions may not work with the code as written
template <class myType>
class SimpleContainer
{
public:
    //I think there are other ways to do the following but I don't know them
    SimpleContainer(int containerSize, myType defaultValue)
    {
        classVector.resize(containerSize);

        for(int i = 0; i < containerSize; i++)
        {
            classVector[i] = defaultValue;
        }
    }

    //Simply looks at the classVector size and returns (used for iterating over it)
    int getSize();

    //The setValue function sets the value at specified container position
    void setValue(int containerPosition, myType inputValue);

    //I feel like I'm missing a reference here but <myType>& threw an error
    void inputEntireVector(std::vector<myType> inputVector);

    //You have to compute the X and Y positions outside of this function if you want
    //to simulate a 2D matrix
    myType getValue (int containerPosition);

    //Prints the entire contents of the container starting from vector.begin()
    //I think this will only work with ints and chars, and with ints over 9 it will
    //mess up the alignment of the console view
    void printContainer();

private:
    std::vector<myType> classVector;
};

template<class myType>
int SimpleContainer<myType>::getSize()
{
    return classVector.size();
}

template<class myType>
void SimpleContainer<myType>::setValue(int containerPosition, myType inputValue)
{
    classVector.erase(classVector.begin() + containerPosition);

    //vector.insert() takes for its third argument a value for the number of times to
    //insert the input value
    int numInputCopies = 1;
    classVector.insert(classVector.begin() + containerPosition, numInputCopies, inputValue);
}

template<class myType>
void SimpleContainer<myType>::inputEntireVector(std::vector<myType> inputVector)
{
    classVector.swap(inputVector);
}

template<class myType>
myType SimpleContainer<myType>::getValue(int containerPosition)
{
    return classVector[containerPosition];
}

template<class myType>
void SimpleContainer<myType>::printContainer()
{
    for(int i = 0; i < classVector.size(); i++)
    {
        std::cout << classVector[i];
    }
}

//Runs some basic interactions with the Container
void sampleContainerInterfacing();

int main()
{
    sampleContainerInterfacing();
    return 0;
}

void sampleContainerInterfacing()
{
    //Setting integers for the view width and height to input into Container
    //Uses them immediately to do the math to figure out Container size
    int width  = CONSOLEWIDTH;
    int height = CONSOLEHEIGHT;
    int containerSize = width*height;

    SimpleContainer<int> mySimpleContainer(containerSize, 0);

    //Outputs one console screen worth of 0's
    mySimpleContainer.printContainer();

    std::cout << mySimpleContainer.getSize() << std::endl;

    //Defining these variables to aid readability of the 2D matrix math
    int position;
    int posY = 5;
    int posX = 7;
    //The position in the container equals the width * the desired y position
    //plus the desired x position, -1 because it starts counting with 0
    position = width * posY + posX - 1;

    mySimpleContainer.setValue(position, 5);
    std::cout << mySimpleContainer.getValue(position) << std::endl;

    //Now contains the input variable
    mySimpleContainer.printContainer();
}

Based on recommendations below I modified the code but I'm getting the following errors.

|8|warning: friend declaration 'myType operator<<(std::ostream&, const SimpleContainer&)' declares a non-template function Note: (if this is not what you intended, make sure the function template has already been declared and add <> after the function name here)

|88|undefined reference to `operator<<(std::ostream&, SimpleContainer const&)'

I know I'm probably making mistakes here. I spent a bunch of time trying to find out how to format this properly but nothing that I found worked. Here is the modified code:

#include <iostream>
#include <vector>
#include <stddef.h>

template<class myType>
class SimpleContainer
{
friend myType operator<<(std::ostream& stream, const SimpleContainer& object);
public:
    SimpleContainer(std::size_t xDim, std::size_t yDim, myType const& defaultValue = myType())
        : objectData(xDim * yDim, defaultValue)
        , xSize(xDim)
        , ySize(yDim)
    {}

    //These are overloaded operators for () for the class which allows for the syntax:
    //Matrix  m(2,3);   // 2D array size 2,3 (default constructed).
    //m(4,5) = 6;       // Will modify the container version to be 6
    myType&       operator()(std::size_t x, std::size_t y)       {return objectData[y*xSize + x];}
    myType const& operator()(std::size_t x, std::size_t y) const {return objectData[y*xSize + x];}

    //Simply looks at the classVector size and returns (used for iterating over it)
    int getSize()
    {
        return objectData.size();
    }

    //Passing by value instead of reference since it uses swap
    void inputEntireVector(std::vector<myType> inputVector)
    {
        objectData.swap(inputVector);
    }

    //Not currently functioning but does compile when not used with std::cout
    void printContainer(std::ostream& stream);

private:
    std::vector<myType> objectData;
    std::size_t          xSize;
    std::size_t          ySize;
};

template<class myType>
inline void SimpleContainer<myType>::printContainer(std::ostream& stream)
{

    for(int i = 0; i < objectData.size(); i++)
    {
        stream << objectData[i];
    }

    // Or you can use std::copy()
    // std::copy(classVector.begin(), classVector.end(),
    //            std::ostream_iterator<type>(stream, ""/*No Space*/));
}

template<class myType>
myType inline operator<<(std::ostream& stream, const SimpleContainer<myType>& object)
{
    object.printContainer(stream);
    return stream;
}

//Runs some basic interactions with the Container
void sampleContainerInterfacing();

int main()
{
    sampleContainerInterfacing();
    return 0;
}

void sampleContainerInterfacing()
{
    //I couldn't figure out how to usefully put these inside the class
    static int const ConsoleWidth  = 80;
    static int const ConsoleHeight = 25;
    size_t width  = ConsoleWidth;
    size_t height = ConsoleHeight;
    SimpleContainer<int> mySimpleContainer(width, height, 0);

    std::cout << mySimpleContainer.getSize() << std::endl;

    mySimpleContainer(0,0) = 5;
    std::cout << mySimpleContainer(0,0) << std::endl;

    //This is the main thing that's not working properly
    std::cout << mySimpleContainer;
}

I had to use the friend function inside the class and use myType in front of the operator<< definition because otherwise the compiler gave the following errors:

|60|error: passing 'const SimpleContainer' as 'this' argument of 'void SimpleContainer::printContainer(std::ostream&) [with myType = int]' discards qualifiers

|58|error: ISO C++ forbids declaration of 'operator<<' with no type

Hopefully these aren't completely stupid questions! Thanks in advance for your time.

Answer 1

Interface:

Even though you are implementing the 2D array as a large 1D array. The user of the array is still expecting to be able to index into the array using the normal array notation.

 Matrix  m(2,3);   // 2D array size 2,3 (default constructed).

 m[1][2] = 4;

You do not want to move the calculation of the position outside the container. If you do this you are exposing implementation details of how the container is built and requiring the user to understand things about the layout of your container. This binds your hands for future modifications and thus breaks encapsulation.

It is technically possible to use the [] to do this. But it is not obvious for beginners (as operator[] can only take 1 parameter) and thus we will use () to simulate the 2D accesses (as this makes the code easier to write).

// The usage of the code will look like this.
// It is not a massive change and people will understand it easily.
// Also the user of the array does not need to know how it is layed out.
m(1,2) = 4;

So your first pass of the interface should look like this:

template <class myType>
class SimpleContainer
{
       std::vector<myType>  data;
       std::size_t          xSize;
       std::size_t          ySize;
    public:
    SimpleContainer(std::size_t xs, std::size_t ys, myType const& defaultValue = myType())
        : data(xs * ys, defaultValue)
        , xSize(xs)
        , ySize(ys)
    {}

    myType&       operator()(std::size_t x, std::size_t y)       {return data[y*xSize + x];}
    myType const& operator()(std::size_t x, std::size_t y) const {return data[y*xSize + x];}
};

Notice I have written two versions of operator(). Both return references to the internal members but one version returns a const reference and is also marked as const.

The normal version:

 myType&       operator()(std::size_t x, std::size_t y)

Allows read/write access to the elements in the array. Because it returns a reference any mutations I do to this object will be reflected in the container copy.

m(4,5) = 6; // Will modify the container version to be 6

The const version:

myType const& operator()(std::size_t x, std::size_t y) const

Allows read only access to the object. This is useful as it allows you to pass your container as a const reference to functions and still access the data. And the compiler will guarantee that the access to the data does not modify it.

If you want to add the ability to do m[1][2] to your container then read this article: http://stackoverflow.com/questions/3755111/how-do-i-define-a-double-brackets-double-iterator-operator-similar-to-vector-of/3755221#3755221

Answer 2

Simple Review of code

In this pass I will do a simple review of the code you have written.
I think there are some problems with the design of your interface but I will deal with those completely separately so this part of the review is solely on the code you have written and assumes that the interface is good.

You don't need to comment why you are including header files.

#include <iostream>    //Needed for std::cout and std::endl
#include <vector>      //Needed for std::vector and others

Don't use macros.

#define CONSOLEWIDTH  80;
#define CONSOLEHEIGHT 25;

These macros have no scope (are all scope depending on how you look at it). Thus you are polluting the namespace. It would be preferable to use int const value. Personally I would make them static members of the class. This allows you to define them inline.

class sampleContainerInterfacing
{
    static int const ConsoleWidth  = 80;
    static int const ConsoleHeight = 25;

The vector already has a constructor that initializes all values:
http://www.sgi.com/tech/stl/Vector.html

Also note sizes are usually defined via std::size_t as this can never be negative.

    //I think there are other ways to do the following but I don't know them
    SimpleContainer(int containerSize, myType defaultValue)
    {
        classVector.resize(containerSize);

        for(int i = 0; i < containerSize; i++)
        {
            classVector[i] = defaultValue;
        }
    }

Try this:

   SimpleContainer(std::size_t size, myType defaultValue)
        : classVector(size, defaultValue)
   {}

This function is simple enough that I would inline it.

    int getSize();

Same comment as constructor. Use std::size_t to indicate a position in the container.

    void setValue(int containerPosition, myType inputValue);

I am not sure I agree with your implementation of setValue(). The call to erase() removes tha value from the array. This will cause the vector to copy all the elements (above containerPosition) down one position (that could be a huge number of copies if the type is not a POD. Then you insert a value back into the location which causes all the elements (from containerPosition) to copied back up 1 position.

template<class myType>
void SimpleContainer<myType>::setValue(int containerPosition, myType inputValue)
{
    classVector.erase(classVector.begin() + containerPosition);

    //vector.insert() takes for its third argument a value for the number of times to
    //insert the input value
    int numInputCopies = 1;
    classVector.insert(classVector.begin() + containerPosition, numInputCopies, inputValue);
}

Personally I would just overwrite the value in the container:

void setValue(int containerPosition, myType inputValue) {classVector[containerPosition] = inputValue;}

Personally I would also pass by (const) reference. The error was probably caused because you did not match the declaration and definition (ie you needed to add the & to both locations).

    //I feel like I'm missing a reference here but <myType>& threw an error
     template<class myType>
    void SimpleContainer<myType>::inputEntireVector(std::vector<myType> inputVector)
    {
        classVector.swap(inputVector);
    }

But you are using swap() on the internal array (which is a valid technique) but requires you to make the copy first so that you don't destroy the original array. What it comes down to is that you need to copy the values from the input into the array. There are two ways to do this.

• Assignment: (Pass parameter by const reference then use assignment)
• Copy and Swap: (Pass parameter by value (thus doing an implicit copy) then call swap()

Same comment as above: Parameter should be std::size_t and its simple enough to put inline.

    //You have to compute the X and Y positions outside of this function if you want
    //to simulate a 2D matrix
    myType getValue (int containerPosition);

Sure: This is fine. But I would pass the stream the the values are printed on as a parameter (thus you are not hard coding it to std::cout).

    //Prints the entire contents of the container starting from vector.begin()
    //I think this will only work with ints and chars, and with ints over 9 it will
    //mess up the alignment of the console view
template<class myType>
void SimpleContainer<myType>::printContainer()
{
    for(int i = 0; i < classVector.size(); i++)
    {
        std::cout << classVector[i];  // Note no space between elements.
    }
}

But if you are going to do that then you should also provide an overload of operator<<(std::ostream& stream, SimpleContainer& data) that calls this method.

Traditionally it is more usual to provide iterators that allow the user to control how much of the array they want to print. Then you can copy the whole array with:

std::copy(con.begin(), con.end(), std::ostream_iterator<type>(std::cout, ", "));

Thus I would write the print like this:

template<class myType>
inline void SimpleContainer<myType>::printContainer(std::ostream& stream)
{
    for(int i = 0; i < classVector.size(); i++)
    {
        stream << classVector[i];  // Note no space between elements (like original)
    }
    // Or you can use std::copy()
    // std::copy(classVector.begin(), classVector.end(),
    //            std::ostream_iterator<type>(stream, ""/*No Space*/));
}

template<class myType>
inline std::ostream& operator<<(std::ostream& stream, impleContainer<myType> const& data)
{
    data.printContainer(stream);
    return stream;
}

Answer 3

I ended up figuring out the required syntax to solve the errors posted in the edit to my original post. Sorry for cluttering up this question, but I wanted to post my code for reference. I ended up figuring out what was wrong by removing the templates and trying to get the code to run with just int instead of myType, and in the end I figured out that for the print function, the compiler didn't want me to pass by const reference. Here is the code:

#include <iostream>
#include <vector>
#include <iterator>
#include <stddef.h>

template<class myType>
class SimpleContainer
{
public:
    SimpleContainer(std::size_t xDim, std::size_t yDim, myType const& defaultValue)
        : objectData(xDim * yDim, defaultValue)
        , xSize(xDim)
        , ySize(yDim)
    {}

    //These are overloaded operators for () for the class which allows for the syntax:
    //Matrix  m(2,3);   // 2D array size 2,3 (default constructed).
    //m(4,5) = 6;       // Will modify the container version to be 6
    myType&       operator()(std::size_t x, std::size_t y)       {return objectData[y*xSize + x];}
    myType const& operator()(std::size_t x, std::size_t y) const {return objectData[y*xSize + x];}

    //Simply looks at the classVector size and returns (used for iterating over it)
    int getSize()
    {
        return objectData.size();
    }

    //Passing by value instead of reference since it uses swap
    void inputEntireVector(std::vector<myType> inputVector)
    {
        objectData.swap(inputVector);
    }

    void printContainer(std::ostream& stream)
    {
        std::copy(objectData.begin(), objectData.end(),
                  std::ostream_iterator<myType>(stream, ""/*No Space*/));
    }

private:
    std::vector<myType> objectData;
    std::size_t          xSize;
    std::size_t          ySize;
};

//Wanted to pass by const reference to the object but it won't compile
template<class myType>
inline std::ostream& operator<<(std::ostream& stream, SimpleContainer<myType>& object)
{
    object.printContainer(stream);
    return stream;
}

//Runs some basic interactions with the Container
void sampleContainerInterfacing();

int main()
{
    sampleContainerInterfacing();
    return 0;
}

void sampleContainerInterfacing()
{
    //I couldn't figure out how to usefully put these inside the class
    static int const ConsoleWidth  = 80;
    static int const ConsoleHeight = 25;
    size_t width  = ConsoleWidth;
    size_t height = ConsoleHeight;

    SimpleContainer<int> mySimpleContainer(width, height, 0);

    std::cout << mySimpleContainer;

    std::cout << mySimpleContainer.getSize() << std::endl;

    mySimpleContainer(0,0) = 5;
    std::cout << mySimpleContainer(0,0) << std::endl;

    std::cout << mySimpleContainer;
}