7
\$\begingroup\$

I wrote an Array class with iterators:

#include <iostream>
using namespace std;
template <class T>
class Array
{
    T* data;
    int size;
public:
    class mException { };
    Array(int nsize=1)try:data(new T[nsize]),size(nsize)
    {

    }
    catch(bad_alloc& r)
    {
        cout<< "Cant allocate";
        throw;
    }
    Array(const Array& rhs):data(new T[rhs.size]),size(rhs.size)
    {
        for(int i=0;i<size;i++)
        {
            data[i] = rhs.data[i];
        }
    }
    Array& operator=(const Array& rhs)
    {
        if(this== &rhs) return *this;
        delete [] data;
        size = rhs.size;
        data = new T[size];
        for(int i=0;i<size;i++)
        {
            data[i] = rhs.data[i];
        }
        return *this;
    }
    T& operator[](int index) throw (mException)
    {
        if(index<0 || index>=size) throw mException();
        return data[index];
    }
    const T& operator[](int index) const throw(mException)
    {
        if(index<0 || index>=size) throw mException();
        return data[index];
    }
    const Array operator++(int) //postfix
    {
        Array temp = *this;
        ++*this;
        return temp;
    }
    Array& operator++()
    {
        for(int i=0;i<size;i++)
            data[i]++;
        return *this;
    }
    friend ostream& operator<<(ostream& out, const Array& rhs)
    {
        out << "(";
        for(int i=0;i<rhs.size;i++)
        {
            out << rhs.data[i];
            if(i<rhs.size-1)
             out <<  " ";
        }
        out << ")" << endl;
        return out;
    }
    friend istream& operator>>(istream& in,Array& rhs)
    {
        cout << "enter " << rhs.size << " digits to fill the array" << endl;
        for(int i=0;i<rhs.size;i++) in >> rhs.data[i];
        return in;
    }

    ~Array()
    {
        delete [] data;
    }
    bool empty() const
    {
        return size==0;
    }
     T front()
    {
        return data[0];
    }
     T back()
    {
        return data[size-1];
    }
    class iterator
    {
        const Array* data;
        int index;
        friend class Array;
    public:
        iterator(const Array* arr,int size): data(arr),index(size) { }
        const T& operator*() const
        {
            return data->data[index];
        }
        const iterator operator++(int)
        {
            iterator temp = *this;
            ++*this;
            return temp;
        }
        iterator& operator++()
        {
            ++index;
            return *this;
        }
        friend bool operator==(const iterator& rhs,const iterator& lhs)
        {
            return !(rhs.index!=lhs.index);
        }
        friend bool operator!=(const iterator& rhs, const iterator& lhs)
        {
            return !(rhs.index==lhs.index);
        }
    };
    iterator begin() const{ return iterator(this,0); }
    iterator end() const{ return iterator(this,size); }
};

I have a few questions:

Should I throw exceptions in the iterator class?

I have exceptions in templates, and I have a catch for every type in main alone, and this could be a problem. Is throwing exceptions from exception (like I did) a good choice?

I know this is not a complete version. This is for educational purposes and I need to know where I did badly.

\$\endgroup\$
8
\$\begingroup\$

Using using namespace std;

Don't. See here: Why is "using namespace std" considered bad practice.

Exception Handling

class mException { };

Please just throw the correct standard exception std::invalid_argument, std::runtime_error and so forth. At the very least, inherit from std::exception. You can find all of the standard exceptions here: cppreference.com.

Array(int nsize=1)try:data(new T[nsize]),size(nsize)
{

}
catch(bad_alloc& r)
{
    cout<< "Cant allocate";
    throw;
}

I've never actually seen this type of try/catch used... If your allocation throws a bad_alloc you may be in an out of memory situation or the block was too large. At any rate it is not the job of your container class to print the message or handle the allocation failure, it is the user's job. Just let the exception fly.

Use Copy & Swap idiom for operator =

Copy and swap is an easy way to implement operator = with strong exception safety. You can read more about it here: What is the copy and swap idiom?.

Throw specification

You rarely need to specify the classes thrown here, just skip them. Also throw std::out_of_range instead.

To reduce code duplication you can implement the mutable (non-const) version using the const version and two const_casts (but not the other way around).

T& operator[](int index) throw (mException)
{
    if(index<0 || index>=size) throw mException();
    return data[index];
}
const T& operator[](int index) const throw(mException)
{
    if(index<0 || index>=size) throw mException();
    return data[index];
}

So this should be:

T& operator[](int index){
    // Being pedagogical here so you see what's going on.
    // The below return statement is equivalent to this.
    // const Array* const_this = const_cast<const Array*>(this);
    // const T& const_ref = (*const_this)[index];
    // return const_cast<T&>(const_ref);
    return const_cast<T&>((*const_cast<const Array*>(this))[index]);
}
const T& operator[](int index) const
{
    if(index<0 || index>=size) throw std::out_of_range("Index out of range!");
    return data[index];
}

Note that it's important that the mutable version calls the const version. Not the other way around.

If you have a mutable instance, then calling a const method is OK. As the operator returns a reference into your mutable instance (even though you're accessing it in a const manner), then casting away the constness is safe.

But if you on the other hand call a mutable function from a const method, then the mutable function might modify your const data and this my friend, is no bueno.

Operator ++

The pre- and post- increment operators don't make sense to me. But I guess you need them for something.

Simplify your logic

This:

    for(int i=0;i<rhs.size;i++)
    {
        out << rhs.data[i];
        if(i<rhs.size-1)
         out <<  " ";
    }

could be:

    for(int i = 0; i < rhs.size - 1; i++) {
        out << rhs.data[i] <<  " ";
    }
    if(rhs.size > 0){
        out << rhs.data[rhs.size-1];
    }

It avoids checking the condition inside of the loop each time. (Although your compiler might already do this).

Input stream operator

It is not the array's job to print the explanation of how to use the input operator, that's up to your user. So this:

friend istream& operator>>(istream& in,Array& rhs)
{
    cout << "enter " << rhs.size << " digits to fill the array" << endl;
    for(int i=0;i<rhs.size;i++) in >> rhs.data[i];
    return in;
}

Should simply be:

friend istream& operator>>(istream& in,Array& rhs)
{
    for(int i=0;i<rhs.size;i++) in >> rhs.data[i];
    return in;
}

As you are using istream it means that you can pass a ifstream pointing to a file on the disk to read from. And it's just silly to be forced to print the instructions each time you read from the file automatically.

Return by reference

Your front() and back() methods should return by reference. And you should also implement const versions of these.

Iterator implementation

A basic pointer satisfies the random access iterator criteria and has specialisations for std::iterator_traits. So you could basically just do this:

typedef T* iterator; 

and be done with it. But as this is a learning experience, lets look at your iterator code.

You are storing the base pointer and the index. You can simply store 1 pointer to the indexed element and increment and decrement the pointer instead, this reduces your code. Also you're missing the required typedefs for an iterator for example you're not specifying what iterator category it is so your iterator is not compatible with many standard library functions. You need these as std::iterator_traits is not specialised for your type. The easiest way to get the right typedefs is to inherit from std::iterator<...> and give the correct type arguments to the template. Note that this only gives you the type defs and no other functionality, you still need to implement the iterator.

You can read more about what requirements are put on iterators here: RandomAccessIterator and on the linked pages.

Also there is an easier way to implement the operator ==/!=:

    friend bool operator==(const iterator& rhs,const iterator& lhs)
    {
        return !(rhs.index!=lhs.index);
    }
    friend bool operator!=(const iterator& rhs, const iterator& lhs)
    {
        return !(rhs.index==lhs.index);
    }

should be:

    friend bool operator==(const iterator& rhs,const iterator& lhs)
    {
        return rhs.index == lhs.index;
    }
    friend bool operator!=(const iterator& rhs, const iterator& lhs)
    {
        return !(rhs == lhs);
    }

Now whenever operator == changes, operator != is automatically updated as it uses operator ==. Clever huh?

The whole thing rewritten with just one pointer should be:

class iterator{
    const T* data;
public:
    iterator(const T* arr): data(arr) { }
    const T& operator*() const
    {
        return *data;
    }
    const iterator operator++(int)
    {
        iterator temp = *this;
        ++*this;
        return temp;
    }
    iterator& operator++()
    {
        ++data;
        return *this;
    }
    friend bool operator==(const iterator& rhs,const iterator& lhs)
    {
        return !(rhs.index!=lhs.index);
    }
    friend bool operator!=(const iterator& rhs, const iterator& lhs)
    {
        return !(rhs.index==lhs.index);
    }
};

iterator begin() const{ return iterator(data); }
iterator end() const{ return iterator(data + size); }

Notice how the iterator class behaves like a restricted pointer? Yepp, you are correct, it basically just wraps a pointer in a pointer-like interface. So you could just as well have used a pointer (like I mentioned at the start, a pointer is a fully fledged random access iterator, I hope the above made it clearer as to why it's OK).

You're also missing operator -> and a lot of other things but I choose to ignore that for now.

Style

For crying out loud, white space is free. Use them! At least put spaces around all operators. For example this:

    for(int i=0;i<size;i++)

should be

    for(int i = 0; i < size; i++)

for readability.

\$\endgroup\$
  • 1
    \$\begingroup\$ std::rethrow_exception(std::current_exception()); is an awful lot of code to write when the original throw; works just fine. There's no need to go through that circumlocution. current_exception() is there to support chaining exception objects and for managing exceptions across threads (for example, std::future needs it). Other than that, excellent answer. \$\endgroup\$ – Pete Becker Aug 27 '15 at 12:35
1
\$\begingroup\$

You may read this discussion for the reference on the exceptions in the iterators.

Meanwhile, your code contains some badly designed pieces. For example, assignment operator is not exception safe. See this discussion

\$\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.