Re: overloading the [] operator

Axter wrote:


I am not posting this as an "example" as I don't agree with the idea.
I am also not posting it as an expert sample. I am posting this to
show some of the things a programmer should do differently than you did
if they insist on going that way and why your code "example" should not
be considered such.

If anyone believes the following code exhibits bad practice in any way
please speak up. I won't promise to agree but do listen and in cases
such as this I believe correctness is more important than my ego.

Note the use of whitespace!!

I did keep my lines to 80 chars but this "newsreader" may still break
the formatting. Does anyone have a suggestion of some place to put
this for better readability?

I don't normally comment so verbosely. In this case explainations and
reasoning are the important part...not necissarily the code itself. As
a coding standard I try to make my code itself do the explaining and
not make use of cryptic style that HAS to be commented.

One question I do have for the experts: In the copy constructor I
originally had new T[colCount_ * rowCount_] but this caused occasional
crashes (that were actually not so occasional). It is my understanding
that rowCount_ and colCount_ are initialized before the call to new T[]
so that should have worked. Using other's variables instead fixed the
problem for at least 20 runs so I am confident it is fixed. The
question is, why did it not work the first time, my failure or
compilers? If mine, where is my understanding flawed?

CODE: ==================================

// This class reimplements the dynamic_2d_array template created
// by David Maisonave and can be viewed at:
// http://code.axter.com/dynamic_2d_array.h
//
// This version fixes some of the bugs and poor coding practices in the
above
// implementation. I do not recommend doing anything like this, but if
you
// must, and want an example, I can't leave you at the mercy of a coder
that
// comes up with the above and calls himself an expert.
//
// Note: I am also not an expert. There are very likely places where
this could
// be improved. My heart is not into it as I disagree with the very
idea.

#include <cstdlib>
#include <cassert>

// Use typename or class? It doesn't really matter. One could
succesfully
// argue that the primary purpose of this class isn't to contain
classes though.
template< typename T >
class dynamic_2d_array
{
public:
  dynamic_2d_array(size_t rowCount, size_t colCount)
    : rowCount_(rowCount), colCount_(colCount),
      storage_(new T[rowCount * colCount])
  {
    /*
     * Changes: more descriptive variable names and a lack for a check
     * of size for 0. Allocate any time. The reason for the
     * later is simple: the check doesn't help, ignores the user's
     * intention, and causes undefined behavior if the class is ever
     * used.
     *
     * The correct way to do what the original code does, making sure
this
     * class isn't used with 0 bounds is:
     */
    assert(rowCount > 0 && colCount > 0);
     /*
      * You could throw an exception instead. All depends on if this
is an
      * incorrect use of the class or a runtime error. I'm choosing
incorrect
      * use of the class. Seems logical and is easier to implement.
      */
  }

  dynamic_2d_array(const dynamic_2d_array & other)
    : rowCount_(other.rowCount_), colCount_(other.colCount_),
      storage_(new T[other.rowCount_ * other.colCount_])
  {
    // In this case there is no need for the check at all as the
creation
    // of a dynamic_2d_array with 0 bounds has been disallowed.
    // Assert anyway to advertise the precondition...

    assert(other.rowCount_ > 0 && other.colCount_ > 0 && other.storage_
!= 0);

    std::copy(other.storage_,
          other.storage_ + (rowCount_ * colCount_),
          storage_);
  }

  ~dynamic_2d_array() { delete [] storage_; }

  // These definitions hide the fact that you are actually returning
  // a pointer to insides. Clients are discouraged from depending on
this fact
  // as it is not advertized by the interface. You could change this
at any
  // time and not affect well behaved clients. Without these
definitions any
  // changes would propegate to all callers.
  typedef T* row_iterator;
  typedef const T* const_row_iterator;

  // Col iterator is much more complex and not in the original
interface.
  // Personally I think the interface is not complete without this
definition
  // but am not willing to spend the time implementing it. For one,
operator[]
  // would then be totally inappropriate and implementing that operator
is the
  // purpose of the original class.

  row_iterator operator [] (size_t row)
  {
    return storage_ + (row * colCount_);
  }

  const_row_iterator operator [] (size_t row) const
  {
    return storage_ + (row * colCount_);
  }

  // That is the extent of the original dynamic_2d_array. Those are
all the
  // messages that object can respond to. Not very dynamic. Here are
some
  // more messages that will let this class at least be useful in that
it will
  // do MORE, not LESS, than a vector or primitive array.

  // bounds checking implementations of the above:
  row_iterator row(size_t row)
  {
    if (!(row < rowCount))
      {
    throw std::out_of_range("Row index out of range");
      }

    return storage_ + (row * colCount_);
  }

  const_row_iterator row(size_t row) const
  {
    if (!(row > rowCount))
      {
    throw std::out_of_range("Row index out of range");
      }

    return storage_ + (row * colCount_);
  }

  // No way to protect the row iterator from invalid col bounds because
of the
  // implementation. Changing row_iterator to a smarter object could
fix that.

  // provide sizing knowledge, needed by almost any client:
  size_t rowCount() const { return rowCount_; }
  size_t colCount() const { return colCount_; }

  // provide resizing - make the class live up to its name:
  void resize(size_t rowCount, size_t colCount)
  {
    assert(rowCount > 0 && colCount > 0);

    T * temp_storage = new T[rowCount * colCount];
    memset(temp_storage, 0, sizeof(T) * rowCount * colCount);

    size_t rows = std::min(rowCount, rowCount_);
    size_t cols = std::min(colCount, colCount_);

    for (size_t i = 0; i < rows; ++i)
      {
    std::copy(storage_ + (i * colCount_),
          storage_ + (i * colCount_) + cols,
          temp_storage + (i * colCount));
      }

    // operations that could cause an exception are finished.
    // None of the following can. Strong guarantee...
    std::swap(storage_, temp_storage);

    rowCount_ = rowCount;
    colCount_ = colCount;

    delete [] temp_storage;
  }

  /*
   * The lack of this operator is a major deficiency of the original
class.
   * At one point it was hidden under the "protected" scope and I
mentioned that
   * this was not the best as the class cannot be overriden anyway.
The
   * original was better at that time but has since been "improved" to
not have
   * or hide this operator. This means it is implemented by default
and will
   * generate undefined behavior the first time it is used and the
objects
   * destroyed. */
  dynamic_2d_array & operator = (dynamic_2d_array other)
  {
    // Strong exception guarantee...
    std::swap(storage_, other.storage_); // If this is new to you look
closely.
    std::swap(rowCount_, other.rowCount_);
    std::swap(colCount_, other.colCount_);

    /* note that 'other' now owns my old storage and will delete it
when this
     * function exits. */
  }

  // To finish the *advertized* interface of the class as commented in
the
  // original we must provide direct access to the data storage for C
functons
  // that need it. This is reasonable but dangerous.
  // Probably better ways to do this:

  T * c_array() { return storage_; } // no reason to pretend we are not
doing
   // what we are doing...

  // Comments should reflect the inherint danger of calling the above
function
  // at the least. Use should be highly discouraged. Better yet,
don't
  // implement it at all and/or do something better. Perhapse the more
costly
  // but more correct way:
  // std::auto_ptr<T> c_array() const;

  /*
   * other useful interface items that could be implemented:
   * * explicit constructor from c array.
   * * functional interface to get T object as f() or operator ().
   *
   * Do not be tempted to implement a non-explicit constructor from a c
array
   * or to implement an implicit conversion to T* or T**.
   */

private:
  T * storage_;
  size_t rowCount_;
  size_t colCount_;
};

#include <iostream>

int main()
{
  typedef dynamic_2d_array<int> int_array;

  int_array a1(5, 7);
  assert(a1.colCount() == 7);
  assert(a1.rowCount() == 5);

  a1[3][5] = 20;
  a1[4][6] = 30;

  assert(a1[3][5] == 20);
  assert(a1[4][6] == 30);

  a1.resize(7, 10);

  assert(a1[3][5] == 20);
  assert(a1[4][6] == 30);

  a1.resize(4, 6);

  assert(a1[3][5] == 20);

  int_array a2(a1);

  assert(a1[3][5] == 20);

  a1[1][1] = 100;

  assert(a1[3][5] == 20);
  assert(a1[1][1] == 100);

  a2 = a1;

  assert(a1[3][5] == 20);
  assert(a1[1][1] == 100);
  assert(a2[3][5] == 20);
  assert(a2[1][1] == 100);

  std::cout << "ALL IS WELL WITH THE INTERFACE.\n";

  return 0;
}