HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Return row or column from a 2D array

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fromcolumnreturnarrayrow

Problem

I have a 2D array class that is actually mapped to a 1D vector. For example, to obtain an element at a particular position, the code is as follows:

template 
T mat ::at(const size_t &row, const size_t &col) const
{
  return m_values[row*ncol() + col];
}


Here's the relevant section of the class in-context:

template 
class mat
{
private:
  std::vector  m_values;
  size_t m_nrow, m_ncol;
public:
  mat();

  size_t ncol() const{ return m_ncol; };
  size_t nrow() const{ return m_nrow; };

  T at(const size_t &row, const size_t &col) const;
  std::vector  row(const size_t &row) const;
  std::vector  col(const size_t &col) const;
};


m_nrow and m_ncol contain the number of rows and columns of the 2D array, respectively, while m_values contains the values of the array. They're assigned in another (long) constructor.

I now need to be able to return an entire row or column. Here's how that's currently implemented:

template 
std::vector  mat ::row(const size_t &row) const
{
  std::vector  tmp;
  for(size_t j=0; j 
std::vector  mat ::col(const size_t &col) const
{
  std::vector  tmp;
  for(size_t i=0; i < nrow(); i++)
  {
    tmp.push_back(at(i, col));
  }
  return tmp;
}


These implementations are easy to reason about but not particularly efficient: there's the pushing back to a temporary vector and the repeated calculation of row*ncol() in at(). Is there a more idiomatic way of doing this in C++11?

Solution

Lack of modifiability

Your at(), row() and col() return copies. This means that once you construct your mat, it's permanently const. Also, for some Ts, this makes these functions unnecessarily expensive.

Prefer instead for at() to return a reference:

T& at(size_t row, size_t col);
T const& at(size_t row, size_t col) const;


Although, typically with the standard library, the at() member functions also do range checking. So consider some other non-throwing indexing mechanism (maybe operator()) and have at() defer to that one while doing bounds checking. Note that you don't need to pass the size_ts by reference-to-const, value is fine.

Now, for the other two, prefer instead:

std::vector>       row(size_t );
std::vector> row(size_t ) const;
std::vector>       col(size_t );
std::vector> col(size_t ) const;


reference_wrapper since you can't have vector.

Constructing those rows/cols

Since you're storing everything in row order, returning a row can just use the iterator-pair constructor of std::vector:

std::vector> row(size_t r)
{
    auto start = m_values.begin() + r * ncol();
    return {start, start + ncol()};
}


Returning a column is much more annoying, but you definitely want to use reserve() to avoid any extra allocations:

std::vector> col(size_t c)
{
    std::vector> result;
    result.reserve(nrows());
    for (size_t r = 0; r < nrows(); ++r) {
        result.push_back((*this)(r, c));
    }
    return result;
}


Where I'm assuming operator() is the non-throwing version of at.

Code Snippets

T& at(size_t row, size_t col);
T const& at(size_t row, size_t col) const;
std::vector<std::reference_wrapper<T>>       row(size_t );
std::vector<std::reference_wrapper<const T>> row(size_t ) const;
std::vector<std::reference_wrapper<T>>       col(size_t );
std::vector<std::reference_wrapper<const T>> col(size_t ) const;
std::vector<std::reference_wrapper<T>> row(size_t r)
{
    auto start = m_values.begin() + r * ncol();
    return {start, start + ncol()};
}
std::vector<std::reference_wrapper<T>> col(size_t c)
{
    std::vector<std::reference_wrapper<T>> result;
    result.reserve(nrows());
    for (size_t r = 0; r < nrows(); ++r) {
        result.push_back((*this)(r, c));
    }
    return result;
}

Context

StackExchange Code Review Q#114658, answer score: 4

Revisions (0)

No revisions yet.