current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

This is a pointer class of mine that I would like to have reviewed. Right now it only allocates in the stack and has no copy or move operations.

template<typename T>
class Ptr {
public:
    Ptr(T* t, int s = 1) :p{t},sz{s} { }
    T& operator*() {
        check_range();
        return *p;
    }
    T& operator[](int i) {
        check_range(i);
        return p[i];
    }
    Ptr& operator+(int i) {
        index += i;
        check_range(index);
        p+= i;
        return *this;
    }
    Ptr& operator-(int i) {
        index -= i;
        check_range(index);
        p -= i;
        return *this;
    }
    Ptr& operator++() {
        *this = *this+1;
        return *this;
    }
    Ptr operator++(int) {
        Ptr<T> old{p};
        ++(*this);
        return old;
    }
    Ptr& operator--() {
        *this = *this-1;
        return *this;
    }
    Ptr operator--(int) {
        Ptr<T> old{p};
        --(*this);
        return old;
    }
private:
    T* p;
    int sz;
    int index = 0;
    void check_range(int i) {
        if (i < 0 || i > sz-1) {
            throw std::out_of_range("out of range");
        }
    }
    void check_range() {
        if (p == nullptr) {
            throw std::out_of_range("null pointer");
        }
    }
};

How can I make this shorter and less ugly? Is there a better solution than what I did?

share|improve this question

1 Answer 1

up vote 7 down vote accepted

I would expect operator+ and operator- to return a new object (leaving the current one intact.

    Ptr& operator+(int i) {
        index += i;
        check_range(index);
        p+= i;
        return *this;
    }
    Ptr& operator-(int i) {
        index -= i;
        check_range(index);
        p -= i;
        return *this;
    }

If I was using normal pointers.

 T* x  = getAT();
 x + 1;           // Does not change x
 T* y = x + 1;    // Does not change x (but y has the value of x+1).

Usually you see operator+ defined in terms of operator+=

Ptr& operator+(int i) {
   Ptr result(*this);
   return result+=1;
}

The *this = *this+1; look very strange.

    Ptr& operator++() {
        *this = *this+1;
        return *this;
    }

I would have just called the operator+=

    Ptr& operator++() {
        return operator+=(1);
    }

Again don't like ++(*this);

    Ptr operator++(int) {
        Ptr<T> old{p};
        ++(*this);
        return old;
    }

I would have used:

    Ptr operator++(int) {
        Ptr<T> old{p};
        operator++();
        return old;
    }

You should strive for the strong exception guarantee (also known as transaction exception guarantee).

    void check_range(int i) {
        if (i < 0 || i > sz-1) {
            throw std::out_of_range("out of range");
        }
    }

Though this code looks like it does that. It is usually called in a way that prevents it.

    index -= i;
    check_range(index);  // If this throws your object is now in the new state
                         // with index out of range. I would have tried to make sure
                         // that if an exception is thrown then the state of the object
                         // remains unchanged.


    // Change to this:

    check_range(index - i); // If it throws then the object is still
                            // in a consistent unchanged state.
    index -= 1;             // If we get here then change the state.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.