3
\$\begingroup\$

I'd like to have a review on my 2D-array container. Any criticism is welcome! I based this on std::array implementation. Hopefully I didn't do too many bad stuff here.

#ifndef 2DARRAY
#define 2DARRAY

#include <iostream>
#include <exception>
#include <initializer_list>
#include <cassert>
#include <stdexcept>

namespace container {
    template<typename T, std::size_t rows, std::size_t columns>

    class array_2d
    {
    static_assert(rows != 0, "error: first array size cannot be 0");
    static_assert(columns != 0, "error: second array size cannot be 0");

    private:
        T m_array2d[rows][columns]{};
    
    public:
        // Aliases
        using size_type = std::size_t;
        using nested_init_list_type = std::initializer_list<std::initializer_list<T>>;
        using pointer = T*;
        using const_pointer = const T*;
        using reference = T&;
        using const_reference = const T&;
        using reverse_iterator = std::reverse_iterator<pointer>;
        using const_reverse_iterator = std::reverse_iterator<const_pointer>;

        // Avoids having to type 3 braces, allows 2 braces only
        // Note: 2 braces are always needed even for one single element
        explicit array_2d(nested_init_list_type array2d_list) {
            assert(array2d_list.size() <= rows && "Wrong number of rows [1st index] inserted");
            size_type row{ 0 };
            size_type column{ 0 };
            for (auto& currentBrace : array2d_list) {
                assert(currentBrace.size() <= columns && "Wrong number of columns [2nd index] inserted"); 
                for (auto& currentValue : currentBrace) {
                    m_array2d[row][column++] = currentValue;
                }
                ++row;
                column = 0;
            }   
        }

        // Accessors (No bound checking)
        constexpr pointer operator[](size_type index)             { return m_array2d[index]; }
        constexpr const_pointer operator[](size_type index) const     { return m_array2d[index]; }
        constexpr reference back() noexcept               { return m_array2d[rows-1][columns-1]; } 
        constexpr const_reference back() const noexcept           { return m_array2d[rows-1][columns-1]; }
        constexpr pointer data() noexcept                 { return m_array2d[0]; }
        constexpr const_pointer data() const noexcept             { return m_array2d[0]; }
        constexpr reference front() noexcept                  { return *(data()); }
        constexpr const_reference front() const noexcept          { return *(data()); }


        // Accessors (With bound checking)
        constexpr reference at(size_type rowIndex, size_type columnIndex) 
        { return (rowIndex < rows && columnIndex < columns) ? m_array2d[rowIndex][columnIndex] : throw std::out_of_range("Error: Index out of range"); }
        constexpr const_reference at(size_type rowIndex, size_type columnIndex) const
        { return (rowIndex < rows && columnIndex < columns) ? m_array2d[rowIndex][columnIndex] : throw std::out_of_range("Error: Index out of range"); }

        // Size related
        constexpr size_type size() const noexcept        { return rows * columns; }
        constexpr size_type row_size() const noexcept    { return rows; }
        constexpr size_type column_size() const noexcept { return columns; }

        // Miscellaneous
        void fill(const size_type& value) noexcept {
            for (auto& element : m_array2d) 
                for (auto& current : element) 
                    current = std::move(value);
        }

        void swap(array_2d& other) noexcept { std::swap(m_array2d, other.m_array2d); }

        // Iterators - Missing reverse iterators
        constexpr pointer begin() noexcept       { return std::begin(m_array2d); }
        constexpr const auto begin() const noexcept  { return std::begin(m_array2d); }
        constexpr const auto cbegin() const noexcept { return std::begin(m_array2d); }

        constexpr auto end() noexcept            { return std::end(m_array2d); }
        constexpr const auto end() const noexcept    { return std::end(m_array2d); }
        constexpr const auto cend() const noexcept   { return std::end(m_array2d); }
    };
}

#endif
\$\endgroup\$
1
  • 1
    \$\begingroup\$ #define 2DARRAY - that's not valid. \$\endgroup\$
    – L. F.
    Dec 23, 2020 at 11:48

1 Answer 1

2
\$\begingroup\$

Preprocessor macro identifiers can't begin with a digit, so the include guard name must be changed.

There's no reason to include <iostream> header.

A common convention for template parameters is to use PascalCase to distinguish them from members and local variables.

Here's a bug:

            for (auto& current : element) 
                current = std::move(value);

The second and subsequent iterations of the loop use the moved-from value, which is probably not what we want. We shouldn't even be able to move from a const ref anyway. Just replace with current = value; or (better!) replace the whole loop with a call to std::fill().

On the other hand, I believe we can safely move from the initializer_list in the constructor. Remember to include <utility> to declare std::move.

Well done for providing the usual type aliases. However, some are missing (e.g. iterator, value_type), and others are inconsistent with the implementation.


I think the real problem you have here is that we're not really clear what the element type is. I recommend making our value_type be T[columns], consistent with our begin() and end(), but also providing a view class onto all the elements as a flat array.

That would look something like

private:
    T m_array2d[rows*columns]{};

public:
    iterator begin() { return m_array2d; }
    iterator end() { return m_array2d + rows*columns; }
    // etc
    T[rows*columns]& data();
    T[rows*columns] const& data() const;

Then fill() becomes much simpler, using std::fill() on the flat view:

void fill(const size_type& value)
{
    using std::begin;  // for argument-dependent lookup
    using std::end;
    auto& d = data();
    std::fill(begin(d), end(d), value);
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your insightful comment. This was really on point and improved the code quite a lot! \$\endgroup\$
    – user235054
    Dec 26, 2020 at 13:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.