5
\$\begingroup\$

I plan to implement a simple simulation in C++, which requires mathematical vectors. The following header file contains the complete vector class with all it's method definitions. By overloading several operators, I tried to make the class more usable. For example: the components of the vector can be modified using v[0] = 5; instead of using v.set(0, 5);.

Nervetheless, the header file seems quite long and the class could probably be rewritten in a modern way. Additionally, some vector operations could parallelized. Any tips for improving the code quality and performance?

Header file:

#ifndef __COLUMNVECTOR_H__
#define __COLUMNVECTOR_H__

#include <vector>
#include <algorithm>
#include <functional>
#include <numeric>
#include <stdexcept>

template <typename T>
class ColumnVector
{
  public:
    const unsigned int DIMENSION;

  private:
    std::vector<T> entries;

  public:
    ColumnVector(unsigned int);
    ColumnVector(const std::vector<T> &);
    T &operator[](unsigned int);
    bool operator==(const ColumnVector<T> &) const;
    bool operator!=(const ColumnVector<T> &) const;
    ColumnVector<T> operator+(const ColumnVector<T> &) const;
    ColumnVector<T> operator-(const ColumnVector<T> &) const;
    ColumnVector<T> &operator+=(const ColumnVector<T> &);
    ColumnVector<T> &operator-=(const ColumnVector<T> &);
    T operator*(const ColumnVector<T> &)const;

  private:
    bool equals(const ColumnVector<T> &) const;
};

template <typename T>
ColumnVector<T>::ColumnVector(unsigned int d) : DIMENSION(d),
                                                entries(d)
{
}

template <typename T>
ColumnVector<T>::ColumnVector(const std::vector<T> &v) : DIMENSION(v.size()),
                                                         entries(v)
{
}

template <typename T>
T &ColumnVector<T>::operator[](unsigned int i)
{
    if (i >= DIMENSION)
    {
        throw std::out_of_range("Not enough dimensions!");
    }
    return entries[i];
}

template <typename T>
bool ColumnVector<T>::equals(const ColumnVector<T> &o) const
{
    return (DIMENSION == o.DIMENSION) &&
           (entries == o.entries);
}

template <typename T>
bool ColumnVector<T>::operator==(const ColumnVector<T> &o) const
{
    return *this.equals(o);
}

template <typename T>
bool ColumnVector<T>::operator!=(const ColumnVector<T> &o) const
{
    return !*this.equals(o);
}

template <typename T>
ColumnVector<T> ColumnVector<T>::operator+(const ColumnVector<T> &o) const
{
    if (DIMENSION != o.DIMENSION)
    {
        throw std::length_error("Dimensions must be equal!");
    }
    ColumnVector<T> result(DIMENSION);
    std::transform(entries.begin(), entries.end(),
                   o.entries.begin(), result.entries.begin(),
                   std::plus<T>());
    return result;
}

template <typename T>
ColumnVector<T> ColumnVector<T>::operator-(const ColumnVector<T> &o) const
{
    if (DIMENSION != o.DIMENSION)
    {
        throw std::length_error("Dimensions must be equal!");
    }
    ColumnVector<T> result(DIMENSION);
    std::transform(entries.begin(), entries.end(),
                   o.entries.begin(), result.entries.begin(),
                   std::minus<T>());
    return result;
}

template <typename T>
ColumnVector<T> &ColumnVector<T>::operator+=(const ColumnVector<T> &o)
{
    if (DIMENSION != o.DIMENSION)
    {
        throw std::length_error("Dimensions must be equal!");
    }
    std::transform(entries.begin(), entries.end(),
                   o.entries.begin(), entries.begin(),
                   std::plus<T>());
    return *this;
}

template <typename T>
ColumnVector<T> &ColumnVector<T>::operator-=(const ColumnVector<T> &o)
{
    if (DIMENSION != o.DIMENSION)
    {
        throw std::length_error("Dimensions must be equal!");
    }
    std::transform(entries.begin(), entries.end(),
                   o.entries.begin(), entries.begin(),
                   std::minus<T>());
    return *this;
}

template <typename T>
T ColumnVector<T>::operator*(const ColumnVector<T> &o) const
{
    if (DIMENSION != o.DIMENSION)
    {
        throw std::length_error("Dimensions must be equal!");
    }
    std::vector<T> multiplied(DIMENSION);
    std::transform(entries.begin(), entries.end(),
                   o.entries.begin(), multiplied.begin(),
                   std::multiplies<T>());
    return std::accumulate(multiplied.begin(), multiplied.end(), 0);
}

#endif

Usage example:

#include <iostream>
#include "ColumnVector.h"

using namespace std;

int main()
{
    ColumnVector<int> cv(std::vector<int>{1, 2, 3});
    ColumnVector<int> cv2(std::vector<int>{1, 2, 3});
    cout << cv * cv2 << "\n" << cv + cv2 << endl;
}
\$\endgroup\$
2
\$\begingroup\$

First I'd like to recommend making the dimension non-const. Changing the dimension is a legal operation, for example when projecting spatial coordinates to the (x, y) plane, or when doing affine transforms and working with homogenous coordinates.

Then... that's the usual approach - "has-a" std::vector<T>. That's reasonable most times, but maybe not this time. Because this way, you will end up rebuilding much of the vector interface... soon you will notice an operator[]() is not enough: you will need a operator[]() const, too, as soon you have const ColumnVector!

As

  1. the operations on std::vector and ColumnVector are easily distinguishable by topic, and

  2. it's virtually impossible to corrupt a ColumnVector if operations like push_back are available (that would just change the dimension - it will still be a valid ColumnVector after that),

I'd recommend to brace yourself and... use "is-a". In other words, to subclass std::vector... This will greatly simplify the interface you will have to write, and you can focus on the mathematical characteristics of ColumnVector. Because in this case, there is no need to hide/shadow/wrap-away all these nice std::vector constructors and members.

\$\endgroup\$
1
\$\begingroup\$
  1. Is there a reason you are duplicating the std::vector's .size()?
    Maybe you should use a std::unique_ptr<T[]> instead of the vector...

  2. It's pretty unusual for fields to have all-uppercase names. That's normally reserved for non-instance constants.

  3. Reduce coupling. That means make it a non-friend non-member if you can.

\$\endgroup\$
1
\$\begingroup\$
  1. You are missing some important constructors, namely move and default constructors. Also some of the operators like operator+ could use move semantics.

  2. Also I would expect an operator *= for multiplying by a scalar

  3. unsigned int should be size_t or just plain unsigned

  4. Your multiplication operator is unnecessarily complicated and wasteful. A simple loop over the elements of both vectors should be sufficient.

\$\endgroup\$
  • \$\begingroup\$ You cannot semantically correctly move from this ColumnVector as DIMENSION is const. Remember that a moved-from object should be in a valid but arbitrary state. \$\endgroup\$ – Deduplicator Jun 14 '17 at 19:50
  • \$\begingroup\$ Thats true, however the purpose of dimension is not really clear to me, other than that it is not a std::vector but rather close to a std::array \$\endgroup\$ – miscco Jun 14 '17 at 19:53
  • 1
    \$\begingroup\$ As for 4, std::inner_product seems to fit the bill. \$\endgroup\$ – vnp Jun 15 '17 at 3:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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