C++ Heap implementation

I'm on my way of understanding basic data structures and sorting algorithms through implementing them in c++. Post is followed of my implementation of Vector. I decided to weld the heap behaviour right into Vector class, although I'm not sure if it's a good solution/practice.
The heapSort method is not yet implemented. I'm not figuring out how I can build it based on already existing methods.
As at my office heavily criticising is an accepted practice, I'm asking for

rather hard, destructive critics and review.

```
#include "Vector.hpp"

Vector::Vector(int size):size(size) {
array = new int[size]();
}

Vector::Vector(int vector[], int sizeT) {
this->array = new int[sizeT]();
this->size = sizeT;
for(int i = 0; i array[i] = vector[i];
}
}

Vector::Vector(Vector* v1) {
this->array = v1->array;
this->size = v1->size;
}

Vector::~Vector() {
delete[] array;
}

int Vector::length() {
return this->size;
}

int Vector::getAt(int idx) {
if(idx >= this->size) {
throw "out of range";
}
return this->array[idx];
}

int& Vector::operator[](const int index){
if(index >= this->size) {
throw "out of range";
}

return this->array[index];
}

void Vector::deteleAt(int idx) {
for(int i = idx; i length() - 1; ++i) {
this->array[i] = this->array[i+1];
}
--this->size;
}

//HEAP--------PRIORITY QUEUE
int Vector::parent(int idx) {
if(idx >= 0 && idx array[0];
}

return this->array[idx/2];
}

bool Vector::hasLeftChild(int idx) {
return this->leftChild(idx) size;
}

bool Vector::hasRightChild(int idx) {
return this->rightChild(idx) size;
}

void Vector::buildMaxHeap() {
for(int i = this->size / 2; i >= 0; i--) {
this->maxHeapify(i);
}
}

void Vector::maxHeapify(int idx) {
if(!this->hasRightChild(idx) && !this->hasLeftChild(idx)) {
return;
}

int l = this->leftChild(idx), r = this->rightChild(idx);
int largest = idx;

if(l size && this-

I'm asking for rather hard, destructive critics and review.

Sure.

Interface.

I don't see the interface (ie class definition). So it becomes harder to review.

Things I don't see

I don't see any move semantics for your class.

Normally (since C++11) I would expect containers to implement move semantics. i.e. I would expect to see the following methods:

Vector(Vector&& move) nothrow
 Vector& operator=(Vector&& move) nothrow
 void swap(Vector& other) nothrow

Also I don't see your code implementing the rule of three. This means that your objects will not work correctly if there are any copies made (and C++ makes copies all the time).

To implement the rule of three you need to implement the following three methods:

Vector(Vector const&);
Vector& operator=(Vector const&);
~Vector();

Code Review

This is a dangerous interface. you are asking the user of your class to know and manually set the size. An important thing in C++ is to try and define interfaces that can not be used incorrectly.

Vector::Vector(int vector[], int sizeT) {

I prefer the iterator version of the interface. Yes it is still just as dangerous but the utility functions add in C++14 will help you correctly identify the dangers and potentially generate compiler errors.

template
Vector::Vector(I begin, I end) {

Now when you use it.

int data[23];
Vector   v(std::begin(data), std::end(data)); // will correctly size the vector
                                              // even if the size is changed.

void test(int* data, std::size_t s)
{
    Vector v(data, data + s); // still works (you just need to add).
}

int*     data = new data[23];
Vector   v(std::begin(data), std::end(data));  // will fail to compile
                                               // For the dangerous
                                               // situations it fails to compile.
                                               // which makes you look.

Use of this-> is discouraged

I consider the use of this-> dangerous as it hides errors. It also shows that you are being lazy in naming your local/member variables.

I know you are going to ask "how does it hide errors?".

The only reason that you need to use this-> is when you have shadowed member variables. You then use this-> to differentiate the member variables from the local variables. But it requires that everybody correctly identify the member/local variable. Any mistake will still silently compile. So this type of error can not be detected by the compiler.

On the other hand if you don't allow shadowed variables (and you can make the compiler detect shadowed variables and fail your compilation) now you have no situation where dropping the this-> (or forgetting) it will NOT cause an error. Since it does not matter whether the this-> is there or not why not just drop it.

This is a funny Constructor.

Not even sure why you would have this interface.

Vector::Vector(Vector* v1) {

You normally have a copy constructor which looks like this:

Vector::Vector(Vector const& v1) {

But an interface that takes pointers is very rare (and for this type of situation is practically never done). You are leaking ownership information. Who ownes the pointer you pass? (Ownership semantics is very important in C++ as the owner of the object is the person responsible for deleting it. A pointer has no ownership information).

Error:

Additionally this interface is broken.

Vector::Vector(Vector* v1) {
    this->array = v1->array;
    this->size = v1->size;
}

Consider this situation:

{
    Vector   a(5);
    Vector   b(&a);   // This is your copy (but the same would apply if
                      // you naively converted it to a standard copy
                      // constructor)
}

In the above case both object point at the same dynamically created object. Not a problem in itself. BUT they both think they own the object and thus will both delete it in there destructor this is illegal.

Const methods

A method that does not change the state of the object should be marked as const.

int Vector::length() {
    return this->size;
}

This is both an indication to the compiler and also allows the object to be used in a context where you only have a const reference (quite common).

Exceptions

Yes you can throw anything. But normally you would throw an exception object that is derived from std::runtime_error.

In your code you are throwing an object of type char const*.

int Vector::getAt(int idx) {
    if(idx >= this->size) {
        throw "out of range";
    }
    return this->array[idx];
}

I would do it like this:

int Vector::getAt(int idx) {
    if(idx >= this->size) {
        throw std::out_of_range("Vector::at()");
    }
    return this->array[idx];
}

Don't do checks for the user.

Don't do uneeded checks for the user. Rather provide methods so that users can perform their own checks to validate there calls. You

StackExchange Code Review Q#147212, answer score: 6