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 2 down vote favorite

I need to write an IntArray implementation for college. I'm wondering if my code could be better and if it is efficient. In the header file are the methods listed that we need to write.

Header file:

 #ifndef INTARRAY_H_
 #define INTARRAY_H_

 #include <iostream>
 using namespace std;

 class IntArray{
 private:
     int length;
     int * data;
public:
    IntArray(int size = 0);
    IntArray(const IntArray& other);
    IntArray& operator=(const IntArray& original);

    int getSize() const { return length; };
    int& operator[](unsigned int i);
    void resize(unsigned int size);
    void insertBefore(int value, int index);

    friend ostream& operator<<(ostream& out, const IntArray& list);

    ~IntArray(){ delete[] data; };

 };
 #endif

Cpp file:

    IntArray::IntArray(int size){
        length = size;
        data = new int[size];
        for(int i = 0;i<size;i++){
             data[i] = 0;
        }
    }
    IntArray::IntArray(const IntArray& other){
        length = other.length;
        data  = new int[length];
        for(int i =0;i<length;i++){
        data[i] = other.data[i];
        }
    }

IntArray& IntArray::operator=(const IntArray& other){
    if(this = &other){
        return *this;
    }
    length = other.length;
    data = new int[length];
    for(int i = 0; i <length;i++){
        data[i] = other[i];
    }
    return *this;
}
void IntArray::resize(unsigned size){
      if (size <= length){
        length = size;
        return;
     }
     int * temparr = new int[size];
     // copy data
     for (int i = 0; i < length; ++i){
        temparr[i] = data[i];
     }
     delete [] data;
     data = temparr;
     length = size;
}

int& IntArray::operator [](unsigned i){
    if(i >= length || i < 0){
        throw "index out of bounds";
    }
    return data[i];
 }

 void IntArray::insertBefore(int d, int index){
     if(index >= length){
         throw "index out of bounds";
     }
     resize(length + 1);
     for(int i = length -1;i>index;i--){
          data[i] = data[i-1];
     }
     data[index] = d;
 }
share|improve this question

closed as off-topic by ratchet freak, Edward, Marc-Andre, nhgrif, Emily L. Aug 11 at 15:37

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – ratchet freak, Edward, Marc-Andre, nhgrif, Emily L.
If this question can be reworded to fit the rules in the help center, please edit the question.
1  
There are some critical bugs: your resize doesn't; the copy will happily go out of bounds and a memory leak when you destroy an instance. –  ratchet freak Aug 11 at 11:00
1  
You're doing an unnecessary cast from unsigned to int. length should be changed to unsigned (all values that can't be negative in the real world should usually be unsigned). –  zenith Aug 11 at 11:02
    
@zenith I have/had the same opinion, but I was told Herb Sutter (and others) disagrees (channel9.msdn.com/Events/GoingNative/2013/…, 44:30). Note for the answer: copy constructor has memory leak (data isn't freed), out of bound in the assignment one and resize method. this-> superfluous in operator[]. No check for size >= 0. –  Armaghast Aug 11 at 13:49
    
I edited the resize. @Armaghast, but if i do 'delete [] data' in copy constructor, will that not cause problems? the program needs the link,not? I also edited the operator[], is that better? –  CPlusPlus Aug 11 at 14:03
    
Are you sure about closing? It looks ontopic to me. –  Caridorc Aug 11 at 21:38

2 Answers 2

up vote 2 down vote accepted

Main changes:

  • Assuming you keep an int length, check for size >= 0 in constructor IntArray(int size = 0);, and in void IntArray::insertBefore(int d, int index)

  • You thought about delete[] data in the destructor. However, there are other places in your code where data needs to be deleted: each time you change data's value, you need to deallocate what used to be pointed by data. Ie, delete[] data before any data =

    1. You added data = new int[length]; in the copy constructor, so you've solved the out of bound issue, but introduced a memory leak instead. You need to deallocate data first.

    2. Same for the assignment constructor.

    3. resize had an out-of-bound issue you've fixed

Code used to be: 

void IntArray::resize(unsigned int size){
    for(int i=length; i<size; i++){
        data[i] = 0;
    }
    length = size;
}

(Note: you're not supposed to edit your code, or put your updated code in a separate section, at the bottom of your question)

Opinion-based

  • Maybe use an unsigned type (like std::size_t) instead of the signed int for the length.

Pros:

  1. You enforce the invariant "the length can only be positive"

Cons:

  1. Special values (like -1 for "Index not found") will look like regular values, higher risk of underflow.
  2. (see Herb Sutter comment at 44:30)

Other

  • Superfluous this-> in operator[](int index)

  • Style: pick one of length or getSize()to get the data length, but don't mix the two (Here length is fine, in classes where you'd like to make a check before accessing the value, getSomething() is better.

  • Add a const version of operator[](int index)

  • Don't add an extra space at the end of operator<< (which you removed by mistake)

Going further

It depends on the assignment.

  • Optimization: Introducing capacity to reduce the number of allocations/deallocations

  • Add iterators

  • Add convenience methods (bool empty() const;, int& front();)

  • Add typedefs like typedef int value_type (useful for for metaprogramming)

  • Add relationship operators (at least == and !=)

@glampert made excellent points I forgot, check his answer.

share|improve this answer
    
Thanks alot, is this a good const version for the [] operator? i will update the rest of the code, thanks it was really useful !!! const int& operator[] (const unsigned i) –  CPlusPlus Aug 11 at 15:57
    
I'd go for const int& operator[] const (unsigned i) , for consistency with the rest of your code. –  Armaghast Aug 11 at 16:02
    
could it be 'const int& IntArray::operator[](unsigned i)const" , otherwise c++ gives a syntax error –  CPlusPlus Aug 11 at 18:22
    
Yes, my mistake, const for the method is after the parameters. –  Armaghast Aug 11 at 23:16
up vote 2 down vote

@Armaghast's answer is already very complete, but there are few other tidbits you should look into:

Don't use using namespace in a header file:

It exposes the whole namespace to any other file #includeing your header file. This pollutes the global scope and can result in name clashes. If you'd like to read a more lengthy discussion, this question on StackOverflow is a good starting point.

Trivia: I just recently noticed that the clang compiler has a -Wheader-hygiene flag, which when provided to the command line, emits a compiler warning if you use namespace in a header file.

Leverage the Standard Library:

I suppose since this is a class assignment, you're not allowed to use std::vector or smart pointers to manage memory for you. If this was not a class assignment, you'd use a std::vector<int> instead. If writing some custom data container/collection, then it would also be better to build it around an existing library class like vector, list, etc, or at the very least, use one of the standard smart pointers to manage memory for you.

That out of the way, there is one library function I think you can use that will make your code better: std::copy(). Avoid using "raw loops" for copying stuff. Here you have two instances of copying the underlaying int array. For example, using std::copy with the resize() method:

void IntArray::resize(unsigned size) {
     ...
     int * tempArr = new int[size];
     std::copy(data, data + length, tempArr);
     delete [] data;
     data = tempArr;
     length = size;
}

Be mindful of indenting and spacing:

Right at the beginning of the implementation file, there is some extra indentation in the code. Be careful to keep indenting neat. It is one of the main things that help reading code.

Also, be careful to properly space operators and keywords. Take this for loop as example, from the copy constructor:

for(int i =0;i<length;i++){

Spacing is a little messy here. It would be more pleasant to the eye if you you'd evenly space each part:

for (int i = 0; i < length; i++) {
share|improve this answer

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