I've been working on a 3D mathematical vector class which should be as streamlined as possible for use in numerical simulations. It will be used to model 3D-physical vectors.

Here, 3D-vector should be taken in mathematical sense, meaning a tuple (a,b,c).

I hoped to design it in a modern and fast way - but one is never perfect. So, I would be interested in some input from your side. Any tips for making this faster?

//threevector.h
#ifndef threevector_h_
#define threevector_h_

#include <fstream>
#include <cmath>
#include <array>


template <class T>
class threevector
{
private:
  static const int dim = 3; //dimension of vector
  std::array<T,dim> container;  

public:

  //constructors and assignment
  threevector(const double a = 0, const double b = 0, const double c = 0):
  container({{a,b,c}}) {}; //standard constructor
  threevector(const threevector& a): container(a.container) {}; 
  //copy constructor

// add once gcc 4.7 is used
//   threevector(threevector&& other): threevector() {swap(*this, other);} 
  // move constructor

  threevector& operator=(threevector rhs) //assignment
  {
    swap(*this, rhs);
    return *this;
  }

  void swap(threevector& first, threevector& second) 
  {first.container.swap(second.container);}


  //operators
  threevector& operator+=(const threevector& rhs) 
  {
    container[0] += rhs.container[0];
    container[1] += rhs.container[1];
    container[2] += rhs.container[2];
    return *this;
  }

  threevector& operator-=(const threevector& rhs) 
  {
    *this += -rhs;
    return *this;
  }

  threevector& operator*=(const double rhs) //scalar multiplication assignment
  {
    container[0] *= rhs;
    container[1] *= rhs;
    container[2] *= rhs;
    return *this;
  }

  threevector& operator/=(const double rhs) //scalar division assignment
  {
    *this *= 1./rhs;
    return *this;
  }


  threevector operator+() const //unary plus
  {
    threevector a(*this);
    return a;
  }

  threevector operator-() const //unary minus
  {
    threevector a(*this);
    a *= -1;
    return a;
  }

  T& operator[](const int input) {return container[input];} //access operator

  const T& operator[](const int input) const {return container[input];} 
  // const access operator

  //utility functions
  double abs() const 
  {return sqrt(container[0]*container[0]+container[1]*container[1]+
   container[2]*container[2]);}
  double abs_sq() const {return pow(abs(),2);}
  void reset() { container[0] = 0; container[1] = 0; container[2] = 0;}
};

//output operator
template<class T>
std::ostream& operator<<(std::ostream& os, const threevector<T>& obj)
{
  os << std::fixed << "(" << obj[0] << "," << obj[1] << "," << obj[2] << ")";
  return os;
}

//addition operator
template<class T>
inline threevector<T> operator+(threevector<T> lhs, const threevector<T>& rhs)
{
  lhs += rhs;
  return lhs;
}

//subtraction operator
template<class T>
inline threevector<T> operator-(threevector<T> lhs, const threevector<T>& rhs)
{
  lhs -= rhs;
  return lhs;
}

//scalar product
template<class T>
inline double operator*(const threevector<T>& lhs, const threevector<T>& rhs) 
{return lhs[0] * rhs[0] + lhs[1] * rhs[1] + lhs[2] * rhs[2];}


//product with scalar
template<class T>
inline threevector<T> operator*(const double lhs, threevector<T> rhs) 
{
  rhs[0] *= lhs;
  rhs[1] *= lhs;
  rhs[2] *= lhs; 
  return rhs;
}

//product with scalar
template<class T>
inline threevector<T> operator*(threevector<T> lhs, const double rhs){return rhs*lhs;}

// scalar division
template<class T>
inline threevector<T> operator/(threevector<T> lhs, const double rhs){return lhs*(1./rhs);};


#endif
up vote 6 down vote accepted

Your design is a little bit confusing. You define your class as a template class, but just about everything takes and returns a double. Generally, you should try and be consistent: will this class work for any numeric-like T? Then make sure it has constructors that take a T instead of a double:

threevector(T a = T(), T b = T(), T c = T())

Similarly for the overloaded operators that multiply/divide by a scalar, and so on.

The name threevector is awkward. At the least it should be three_vector, but that's still a little awkward. vector_3d is more of the "standard" name for this kind of thing.

Your include guards should generally be in all-caps. This is because they are global in scope, and you want to do as much as possible to minimize the possibility of potential name clashes.

#ifndef threevector_h_ // should be THREE_VECTOR_H_
#define threevector_h_ // or VECTOR_3D_H_

You have some semi-colons where they aren't needed:

threevector(const double a = 0, const double b = 0, const double c = 0):
container({{a,b,c}}) {}; //standard constructor
threevector(const threevector& a): container(a.container) {}; // <--- Not needed

Comments like //standard constructor are obvious and don't really need to be there. Further, your copy constructor and copy assignment operator are redundant; the compiler generated ones are sufficient.

It's often easiest to write operator+ (and -, * etc) in terms of operator+=:

vector_3d operator+(const vector_3d& a, const vector_3d& b)
{
     vector_3d result(a);
     result += b;
     return result;
}

For something this simple, it's unlikely you'll be able to speed it up much. The only potential that I can see is replacing the call to pow() in:

double abs_sq() const {return pow(abs(),2);}

with something like:

T abs_sq() const
{
    const T ab = abs();  // abs() should also return a T
    return ab * ab;
}

which avoids the overhead of a function call to pow. In practice, this is unlikely to make much of a difference, however.

  • "Further, your copy constructor and copy assignment operator are redundant; the compiler generated ones are sufficient." Are you sure that the compiler knows about copy-and-swap? – user3058865 Mar 14 '14 at 9:46
  • 1
    In this case, it doesn't need to. You only really need to use copy-and-swap with a resource managing class. Your class doesn't manage any resources (that is, anything that is directly newed), hence copy-and-swap is redundant. – Yuushi Mar 14 '14 at 12:23
  • Many of these comments are pointless. They should only be needed if something is not obvious and needs explanation. There's no need for individual comments for each constructor and operator; these are already noticeable to those familiar with C++.

  • There's no need for inline here. Code in headers are automatically inline, so this would apply to all the code here. Having it here anyway wouldn't cause a problem as the compiler could just remove it, but I'd remove them anyway.

  • dim could just be renamed to dimensions, so that you won't need a comment for it:

    static const int dimensions = 3;
    
  • This is not too readable:

    void swap(threevector& first, threevector& second) 
    {first.container.swap(second.container);}
    

    This would also reduce maintainability, especially if additional lines will need to be added. Using your existing curly brace style, you'll have this:

    void swap(threevector& first, threevector& second) 
    {
        first.container.swap(second.container);
        // now you can easily add additional lines
    }
    

    I'd also recommend keeping this consistent throughout the code. You already use it some places (with the multi-line functions), but this should also be done with the single-line functions.

  • For statements like these:

    threevector& operator-=(const threevector& rhs) 
    {
        *this += -rhs;
        return *this;
    }
    

    you can make it a single line:

    threevector& operator-=(const threevector& rhs) 
    {
        return *this += -rhs;
    }
    
  • I'd use some parenthesis in the scalar product for clarity:

    {
        return (lhs[0] * rhs[0]) + (lhs[1] * rhs[1]) + (lhs[2] * rhs[2]);
    }
    

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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