3
\$\begingroup\$

I would like to learn how to create useful and easy to use for clients Array class. The class below is solely for learning purposes.

Here are the points I am interested in:

  • Style (the code is easy to read and maintain)
  • Logical correctness (class user will get what he\she expects)
  • Efficiency (no very very big performance problems)

I refactored the class as good as I could, so I am asking for help which will taught me.

//Developed by Trofimov Yaroslav on 30.03.2018

#ifndef _ARRAY_H_TROFIMOV_
#define _ARRAY_H_TROFIMOV_

#include <string>

template<const size_t n, typename T>
class Array {
public:
    typedef const bool (* const BooleanResultDelegate)(const T&);
    class ArrayError {
        const std::string _reason;
        const size_t _index;
        const size_t _maxIndex;
    public:
        ArrayError(const size_t index, const size_t maxIndex,const std::string& reason = "") 
            : _index(index), _maxIndex(maxIndex), _reason(reason) {}
        std::string explanation(void) {
            std::string res;
            res += "Index: " + std::to_string(_index) + "\n"; 
            res += "Max index: " + std::to_string(_maxIndex) + "\n"; 
            res += "Reason: " + _reason + "\n"; 
            return res;
        }
    };

private:
    typedef void (Array<n, T>::* const VoidResultDelegate)(const size_t, const BooleanResultDelegate);

    static unsigned __freeId, __quantity;
    unsigned _id;

    T** _array;
    const size_t _n;

    void iterateAndApply(
        const VoidResultDelegate function, 
        const BooleanResultDelegate shouldApply) {
        for(size_t i(0); i < length(); ++i) {
            (this->*function)(i, shouldApply);
        }
    }
    void removeElement(const size_t i, const BooleanResultDelegate removeCondition) {
        if(removeCondition == 0 || 
            (_array[i] != 0 && removeCondition(*_array[i]))) {
                delete [] _array[i]; _array[i] = 0;
        }
    }
    T _replacer;
    void replaceElement(const size_t i, const BooleanResultDelegate replaceCondition) {
        if(replaceCondition == 0 || 
            (_array[i] != 0 && replaceCondition(*_array[i]))) {
                delete [] _array[i];
                _array[i] = new T(_replacer);
        }
    }

public:
    explicit Array<n, T>(T* arrayFiller = 0) 
        : _n(n), _array(new T*[n]), _id(++__freeId) {
            if(arrayFiller != 0) {
                for(size_t i(0); i < length(); ++i) {
                    _array[i] = new T(*arrayFiller);
                }
            } else {
                for(size_t i(0); i < length(); ++i) {
                    _array[i] = arrayFiller;
                }
            }
            reportIfDebug<n, T>(*this, "created");
            ++__quantity;
    }
    explicit Array<n, T>(const T& arrayFiller) 
        : _n(n), _array(new T*[n]), _id(++__freeId) {
            for(size_t i(0); i < length(); ++i) {
                _array[i] = new T(arrayFiller);
            }
            reportIfDebug<n, T>(*this, "created");
            ++__quantity;
    }
    Array<n, T>(const Array<n, T>& that) 
        : _n(n), _array(new T[n]), _id(++__freeId) {
            for(size_t i(0); i < length(); ++i) {
                (*this)[i] = new T[that[i]];
            }
            reportIfDebug<n, T>(*this, "created");
            ++__quantity;
    }
    ~Array<n, T>(void) {
        removeAll();
        delete [] _array; _array = 0;
        reportIfDebug<n, T>(*this, "deleted", false);
        --__quantity;
    }

    T* operator[](const size_t i) {
        if(i > length()) {
            throw ArrayError(i, _n, "out of bounds exception");
        }
        return _array[i];
    }
    const T* operator[](const size_t i) const {
        if(i > length()) {
            throw ArrayError(i, _n, "out of bounds exception");
        }
        return _array[i];
    }
    const size_t length() const {
        return _n;
    }
    const unsigned getID() const {
        return _id;
    }

    void removeAll(const BooleanResultDelegate removeCondition = 0) {
        iterateAndApply(&Array<n, T>::removeElement, removeCondition);
    }
    void replaceAll(const T& replacer, const BooleanResultDelegate replaceCondition = 0) {
        _replacer = replacer;
        iterateAndApply(&Array<n, T>::replaceElement, replaceCondition);
    }
};

template<const size_t n, typename T>
unsigned Array<n, T>::__freeId = 0;
template<const size_t n, typename T>
unsigned Array<n, T>::__quantity = 0;

template<const size_t n, typename T>
void reportIfDebug(
    const Array<n, T>& instance, 
    const char* const message, 
    const bool showContent = true) {
#ifndef NDEBUG
    std::cout << "========================================" << std::endl;
    std::cout << typeid(instance).name() << ' ' 
        << message << ' '
        << "id: " << instance.getID() << std::endl;
    if(showContent) {
        std::cout << instance;
    }
    std::cout << "========================================" << std::endl;
#endif
}
template<const size_t n, typename T>
std::ostream& operator<<(std::ostream& os, const Array<n, T>& instance) {
    for(size_t i(0); i < instance.length(); ++i) {
        if(instance[i] == 0) {
            os << "[" << i << "]: " << instance[i] << "\n";
        } else {
            os << "[" << i << "]: " << *instance[i] << "\n";
        }
    }
    return os;
}
#endif
\$\endgroup\$
4
  • \$\begingroup\$ Sorry, my fault. Your code compiles now (with small fixes): rextester.com/NPARK94793 \$\endgroup\$ Mar 31 '18 at 12:43
  • \$\begingroup\$ Does this only compile in visual studio? Might want to add a remark/tag if that's the case. \$\endgroup\$
    – yuri
    Mar 31 '18 at 13:07
  • \$\begingroup\$ @yuri, I've got a mix of errors and warnings on latest gcc and clang on wandbox. You can copy the example provided by Panta. Link to Wandbox. \$\endgroup\$ Mar 31 '18 at 13:12
  • \$\begingroup\$ @yuri It seems so \$\endgroup\$ Mar 31 '18 at 13:14
1
\$\begingroup\$

Include Guard

Firstly, the include guard is not what I'd opt for in modern C++ and there are some downsides since sometimes your #define may collide (rare but not impossible). Use #pragma once. It's also less boilerplate code.


Type Aliasing

Prefer converting typedef to a using BooleanResultDelegate = ... if you'd like to conform with "modern C++" but it makes no difference here except syntactic.


Exceptions

The standard library already provides an exception analogous to ArrayError which is std::out_of_range. You are already incorporating the indices in the string message itself, so it makes sense to use this exception, since you are not exposing the scalars in the class.

If this is not satisfactory to you, then persist with building your own type but inherit from std::logic_error appropriately. The closer you are to STL behaviour the better, because that is what most C++ users are familiar with.

Furthermore, taking in mind the context your code will be used, are there opportunities to add a noexcept? Remember - following C++ core guideline E.12, it should be used not only when a function doesn't throw, but also if throwing is essentially unrecoverable or nonsensical.

If you encounter an out of bounds error, and you can't even construct the exception object, ArrayError because you have no space for two scalars and a string, does it make sense for the program to continue? If not, perhaps opt for noexcept for the constructor?


Const Correctness

You have places with code like f(const size_t x) and it does not make sense to make it const since it is passed by value, and thus writing to a copy of the variable does not affect the caller, and remember const is like a contract you agree to with the caller.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ I disagree abou your statement on include guards. While include guards are ugly, they are also the only standard conforming way of avoiding double inclusion. #pragma once is non-standard, and opting to not use it is fine imo. Also, I'd say that making variables passed const is fine to ensure implementation correctness. Even though that const does not affect the calling code in any way, it helps avoid mutating local arguments when it's not intended and serves as a basically-free compile time correctness check. \$\endgroup\$ Apr 1 '18 at 15:55
  • \$\begingroup\$ @BenSteffan well, I did use const that way with pass by value in another review and I got a massive backlash in the comments. I guess it can go both ways. \$\endgroup\$
    – JNS
    Apr 1 '18 at 16:09
  • \$\begingroup\$ Interesting. Well, everybody has their own opinion; maybe it's best to just keep these things out of code reviews altogether. \$\endgroup\$ Apr 1 '18 at 16:19

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.