2
\$\begingroup\$

I am working on a very simple 3D vector class, which I can use to set up and access three-dimensional vectors using a wrapper around std::vector. It's not intended to be foolproof, just good enough to get by.

The main goal is to make it simple to use, explicit and easy to debug. I have often accidentally put in out-of-bounds coordinates or given invalid dimensions, so this at least can catch those cases and report back in an understandable way.

Hence all the debug prints, should it be used improperly for any reason.

The only thing I am unhappy with is resize. I would not have included it (too dangerous), but found some cases in my code where I could not set up its dimensions in advance, like if it was a member of another class. In those cases, it needed a default and resize option.

Since resize does all the checking, I just reused it for the constructor rather than copy-paste all that.

I'm still somewhat new to C++ and was taught from a C background, so I'm always looking to improve my style.

template <typename T>
class Vector3D
{
 private:
  std::vector<T> data;

 public:
  // Dimensions in each direction
  unsigned long xDim;
  unsigned long yDim;
  unsigned long zDim;

  // Constructor
  Vector3D(unsigned long _xDim, unsigned long _yDim, unsigned long _zDim)
  {
    resize(_xDim, _yDim, _zDim);
  }

  T& at(unsigned long _x, unsigned long _y, unsigned long _z)
  {
    if (_x >= xDim || _y >= yDim || _z >= zDim)
      {
        std::cerr << "Position: " << x << ", " << y << ", " << z << std::endl;

        std::cerr << "Vector width: " << xDim << ", " << yDim << ", " << zDim
                  << std::endl;

        std::cerr << "Invalid indices into 3D vector" << std::endl;

        // Throw an exception here..
      }

    unsigned position = _x + (_y * xDim) + (_z * xDim * yDim);

    return data[position];
  }

  /**
   * @brief Default constructor. Dangerous to use, should use the explicit one
   * if at all possible.
   */
  Vector3D()
  {
    xDim = 0;
    yDim = 0;
    zDim = 0;
  }

  /**
   * @brief Re-sizes a 3D vector with new dimensions.
   * @details This is a nuclear option. You're re-sizing the dimensions of a
   * three-dimensional vector. Do not expect results to be good. Only use this
   * explicitly when initialization is impossible up-front.
   * @param _xDim, _yDim, _zDim - The new dimensions of the vector
   */
  void resize(unsigned _xDim, unsigned _yDim, unsigned _zDim)
  {
    if (_xDim == 0 || _yDim == 0 || _zDim == 0)
      {
        // Throw an exception here...
      }

    xDim = _xDim;
    yDim = _yDim;
    zDim = _zDim;

    // Resize could fail, especially if the dimensions are too large. Just print
    // out the dimensions in x,y,z, number of elements and total bytes just in
    // case
    try
      {
        data.resize(xDim * yDim * zDim);
      }
    catch (std::exception& e)
      {
        std::cerr << e.what() << std::endl;

        std::cerr << "Error allocating 3D Vector. Dimensions: ";

        std::cerr << "X: " << xDim << " Y: " << yDim << " Z: " << zDim
                  << std::endl;
        std::cerr << "Number of elements: " << (xDim * yDim * zDim)
                  << std::endl;
        std::cerr << "Size in bytes: " << (xDim * yDim * zDim) * sizeof(T)
                  << std::endl;

        // Throw an exception here...
      }
  }

  unsigned long getSize() { return data.size(); }

  // Should only be used if the vector is storing pointers to some data
  void freeData()
  {
    for (unsigned long i = 0; i < data.size(); i++)
      {
        free(data[i]);
      }
  }

  // A couple methods to get the real vector, if they want it.
  std::vector<T>& getData() { return data; }

  const std::vector<T>& getData() const { return data; }
};
\$\endgroup\$
2
  • \$\begingroup\$ throw an exception here :) \$\endgroup\$
    – slepic
    Dec 4, 2019 at 21:52
  • \$\begingroup\$ In my own code there's a macro that calls some home-made assertion class, I didn't want to fiddle with all that but I also wasn't sure what it was doing under the hood, so I left it out. \$\endgroup\$ Dec 4, 2019 at 22:05

2 Answers 2

1
\$\begingroup\$

Quick comments:

  1. unsigned long => size_t everywhere ... it's the same, but that's the alias for this job
  2. check for _xDim, _yDim, _xDim < 0 ... underflow is also undefined behvaiour ;-)
  3. implement operator[] for those who are confident about their bounds, or for faster release builds. It's not trivial for multiple dims, check here.
  4. free data is flawed. You need to look into unique_pointer or shared pointer which will do this stuff for you.
  5. Resize? It is actually required? Can you not auto-resize during at()? unless you want that as a code check during development.
  6. Consider a library
\$\endgroup\$
6
  • \$\begingroup\$ Since the dimensions are passed as unsigned values, checking if they are < 0 will always return false, right? Also, I'm confused on what you meant be resize(). I ran into cases where (for like if I had a Vector3D as a member of another class) it could not be initialized with its dimensions. So I needed to leave a default constructor for it as size zero, then a resize for those cases. I've heard of Eigen, but it's way too large and complex for my needs. I'd prefer to hand-roll if possible. What did you mean by unique or shared pointers, how would that help in this case? \$\endgroup\$ Dec 5, 2019 at 17:25
  • \$\begingroup\$ Also how does operator[] allow "faster release builds"? \$\endgroup\$ Dec 5, 2019 at 17:26
  • \$\begingroup\$ @TylerShellberg Hmm, yes. Re unsigned > 0; You're right, sort of. But ... well I guess that's why they say don't do arithmetic with (which you no doubt will be in the calling code) and then force the result into an unsigned type to "eliminate negatives". This is a complex problem. Be careful. It's quite possible that your calling code will accidentally produce a negative which then gets cast into goodness know what in the unsigned _x, _y, _z..etc \$\endgroup\$ Dec 5, 2019 at 21:23
  • \$\begingroup\$ @TylerShellberg Re operator[]: On std::vector this is faster than ::at() because it is not bounds checked. You might want to model it the same way... or used checks with a #define NDEBUG type flag for Debug builds only. \$\endgroup\$ Dec 5, 2019 at 21:27
  • \$\begingroup\$ @TylerShellberg Re resize: My point was: "Do you really want to resize manually"? It seems weird to me. Either the vector3D has a fixed size which is know on construction or it doesn't. If you don't know the size at time of Default Construction as member of another class, then don't default construct it in that other class. It's hard to say what the right solution to that is without seeing the application, but the time, purpose and scope of use will come into it. \$\endgroup\$ Dec 5, 2019 at 21:34
1
\$\begingroup\$

getSize seems a bit redundant (since you provide a way to get to the underlying vector, and that can be used to get the size). Since Vector3D doesn't have one size, what would that number mean?

Add a way to get all 3 dimensions.

The getData methods are dangerous, since they give access to the underlying vector that would allow the user to change the size of it.

If you are concerned about the dangers of having a resize function, you could have resize throw an exception if the Vector3D already has a size (data is not empty). Then possibly rename it to setsize or init.

\$\endgroup\$

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.