Вклад просил для лучшего программирования - PullRequest
2 голосов
/ 09 февраля 2010

Изучая C ++, я начал реализовывать некоторые распространенные структуры данных как форму практики. Первым был стек (это было первое, что пришло в голову). Я немного программировал, и это работает, но теперь мне нужно немного узнать, что мне делать иначе. Как удаление определенных вещей или других профессиональных советов. Что мне делать иначе и почему?

template <class T>
class Stack
{
private:
    int* values;
    int capacity;
    int itemsOnStack;

public:

    /////////////////// 
    Stack()
    {
        Stack(32);
    }

    /////////////////// 
    Stack(const int sz)
    {
        values = new T[sz];
        capacity = sz;
        itemsOnStack = 0;
    }

    ~Stack()
    {
        values = 0;
               // delete?
    }

    ////////////////////
    void Push(const T& item)
    {   
        *(values + itemsOnStack) = item;

        itemsOnStack++;

        if(itemsOnStack > capacity)
        {
            capacity *= 2;

            T* temp = new T[capacity];
            temp = values;
            values = new T[capacity]; 
            values = temp;          
        }
    }

    /////////////////// 
    T Pop()
    {
        if(itemsOnStack > 0)
        {
            int current = --itemsOnStack;
            return *(values + current);
        }
        return NULL; // ? good?
    }

    /////////////////// 
    T Peek()
    {
        if(itemsOnStack > 0)
        {
            int current = itemsOnStack - 1;
            return *(values + current);
        }
        return NULL; // find something better here or shouldnt?
    }

    /////////////////// 
    int Count()
    {
        return itemsOnStack;
    }

    /////////////////// 
    int Capacity()
    {
        return capacity;
    }

    /////////////////// 
    bool IsEmpty()
    {
        return itemsOnStack == 0;
    }
};

Ответы [ 6 ]

7 голосов
/ 09 февраля 2010

Сделал первый проход исправлений в вашем коде:

template <class T>
class Stack
{
private:
    int* values;
    int capacity;
    int itemsOnStack;

public:

    //Stack() : 
    //{
    //   Stack(32); // doesn't do what you expect. This would create an unnamed temporary stack object
    //}

    Stack(const int sz = 32) // C++ doesn't yet have delegating constructors. You can't call one ctor from another. But in this case, a simple default parameter can be used instead
   : values(new T[sz]), capacity(sz), itemsOnStack() {} // use the initializer list for initializing members

    ~Stack()
    {
        delete[] values; // you allocated it, so you delete it as well
    }

    ////////////////////
    void Push(const T& item)
    {   
        values[itemsOnStack] = item; // cleaner syntactically than your version
     //   *(values + itemsOnStack) = item;

        ++itemsOnStack; // prefer pre-increment by default.

        if(itemsOnStack > capacity) // you need to check this before writing the element. Move this to the top of the function
        {
            int newCapacity = capacity * 2;

            // what's this supposed to do? You're just copying pointers around, not the contents of the array
            T* temp = new T[newCapacity ];
            std::copy(values, values+capacity, temp); // copy the contents from the old array to the new one
            delete[] values; // delete the old array
            values = temp; // store a pointer to the new array
            capacity = newCapacity;
        }
    }

    /////////////////// 
    T Pop()
    {
        T result = Peek(); // you've already got a peek function. Why not use that?
        --itemsOnStack;
        return result;
    }

    /////////////////// 
    T Peek()
    {
        if(itemsOnStack > 0)
        {
            int current = itemsOnStack - 1;
            return values[current]; // again, array syntax is clearer than pointer arithmetics in this case.
        }
//        return NULL; // Only pointers can be null. There is no universal "nil" value in C++. Throw an exception would be my suggestion
          throw StackEmptyException();
    }

    /////////////////// 
    int Count()
    {
        return itemsOnStack;
    }

    /////////////////// 
    int Capacity()
    {
        return capacity;
    }

    /////////////////// 
    bool IsEmpty()
    {
        return itemsOnStack == 0;
    }
};

Осталось исправить:

Копировать конструктор и оператор присваивания. На данный момент, если я попытаюсь сделать это, он ужасно сломается:

Stack<int> s;
Stack<int> t = s; // no copy constructor defined, so it'll just copy the pointer. Then both stacks will share the same internal array, and both will try to delete it when they're destroyed.
Stack<int> u;
u = s; // no assignment operator, so much like above, it'll blow up

Const правильность:
Разве я не могу позвонить Peek() или Count() на const Stack<T>?

Срок службы объекта:
Удаление элемента из стека не вызывает деструктор элемента. Выдвижение элемента не вызывает конструктор элемента. Простое расширение массива вызывает конструктор по умолчанию сразу для всех новых элементов. Конструктор должен вызываться, когда пользователь вставляет элемент, а не раньше, и деструктор сразу после удаления элемента.

И, ну, правильное тестирование:
Я не компилировал, не запускал, не проверял и не отлаживал это каким-либо образом. Поэтому я, скорее всего, пропустил некоторые ошибки и представил несколько новых. ;)

4 голосов
/ 09 февраля 2010

Чтобы назвать несколько наряду с тем, что отметил Джон:

1.) Использовать списки инициализации
2.) При необходимости используйте функции-члены const.
3.) Используйте имена функций, более точно соответствующие стандарту. (пусто () вместо IsEmpty ())
4.) У вас огромные утечки памяти. Уничтожь память в своем деструкторе

3 голосов
/ 10 февраля 2010

У вас много утечек памяти, особенно если вы копируете, назначаете или помещаете объект Stack. Даже в качестве учебного упражнения я бы предложил избегать самостоятельного управления памятью (это то, что вам нужно изучать в C ++, но делать по одному шагу за раз). Вместо этого используйте std :: vector или std :: list вместо вашего необработанного массива значений в стиле C. Это будет означать, что деструктор, конструктор копирования, оператор присваивания и метод Push () не будут пропускать память - по умолчанию они также будут делать правильные вещи (я называю это Принцип наименьшего сюрприза ) , Я думаю, что одна из самых важных частей изучения C ++ - это знание того, как использовать STL. Сначала поймите это и позаботьтесь о указателях и новых [], а затем удалите [].

Я бы предложил прочитать конструкторы копирования, операторы присваивания и деструкторы, чтобы вы знали, как на самом деле копируются и удаляются значения. Попробуйте написать класс:

class Noisy {
  public:
    Noisy() { std::cout << "Constructor" << std::endl; }
    ~Noisy() { std::cout << "Destructor" << std::endl; }
    Noisy& operator=(const Noisy& other) {std::cout << "Assign" << std::endl; }
    Noisy(const Noisy& other) {std::cout << "Copy constructor" << std::endl; }
};

, а затем выполните несколько операций над Stack<Noisy>:

Stack<Noisy> stack(10);
stack.Push(Noisy());
Stack<Noisy> otherStack(stack);
Stack<Noisy> thirdStack;
thirdStack=stack;

и посмотрите, совпадают ли ваши ожидания относительно того, что, по вашему мнению, должно происходить, с тем, что вы видите в окне консоли.

1 голос
/ 09 февраля 2010

Вы, вероятно, должны удалить values в деструкторе, а не просто установить его в 0;

в вашем методе push вы не можете изменить размер такого массива.

больше информации по изменению размеров массивов: http://www.daniweb.com/forums/thread13709.html#

0 голосов
/ 10 февраля 2010

вам нужно принять соглашение об именах четырех переменных-членов. Мы используем

 int *m_values;
 int m_capactity;

и т.д.

А зачем ты это делаешь

 *(values + itemsOnStack) = item;

Полагаю, вы имеете в виду

 values[itemsOnStack] = item;

они делают то же самое, но первое гораздо более естественно

0 голосов
/ 09 февраля 2010

Push(): Вы должны проверить размер массива перед записью в него

Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...