[CR] Implementacja generycznego dynamicznego LIFO

Question

Hej, mógłbym prosić o Code Review, wskazanie dobrych praktyk? Uczę się C++ od niedawna i nie chciałbym powielać swoich błędów.

#pragma once

#include <iostream>

template <class T>
struct stackItem
{
  T value;
  stackItem *previousItemPointer;
};

template <class T>
class stack
{
  stackItem<T> *head;
  void removeLast();

public:
  stack()
  {
    stackItem<T> *head = new stackItem<T>;
    head->previousItemPointer = nullptr;
    this->head = head;
  }

  ~stack()
  {
    delete head;
  }

  void push(T value);
  T pop();
  bool isEmpty();
};

template <class T>
T stack<T>::pop()
{
  T value = head->value;
  removeLast();
  return value;
}

template <class T>
void stack<T>::push(T value)
{
  stackItem<T> *newHead = new stackItem<T>;

  if (newHead)
  {
    newHead->value = value;
    newHead->previousItemPointer = head;
    head = newHead;
  }
  else
  {
    std::cout << "Stack overflow";
  }
}

template <class T>
bool stack<T>::isEmpty()
{
  return head->previousItemPointer == nullptr;
}

template <class T>
void stack<T>::removeLast()
{
  if (isEmpty())
  {
    std::cout << "Stack underflow";
  }
  else
  {
    stackItem<T> *tmpHead = head;
    head = tmpHead->previousItemPointer;
    delete tmpHead;
  }
}

Comments

Tak pobieżnie mam dwie uwagi i dwa pytania.

isEmpty() może być const, oraz metody mogą być z operatorem noexcept(nie widzę byś rzucał wyjątki).

Dlaczego używasz class, zamiast typename?

Dlaczego nazwy klas z małej?

---

Dlaczego nazwy klas z małej?

Sugerowałem się implementacją std::stack. Struktury rozuimiem również należy pisać wielką literą?

---

Dlaczego używasz class, zamiast typename?

To jest dobre pytanie, niestety mam bardzo ingorancką odpowiedź - ponieważ słowo kluczowe 'class' ma mniej zanków do napisania. Nie rozumiem różncy między nimi, a ze źródeł wydedukowałem, że w moim przypadku nie ma znaczenia, czy użyję class, czy typename.

---

metody mogą być z operatorem noexcept(nie widzę byś rzucał wyjątki).

Tak jest! (Czy zamiast cout "stack overflow" powinienem rzucać wyjątkiem?)

---

isEmpty() może być const

Dodałem const! - Rzeczywiście nic nie zmieniam

Dzięki za komentarz! : )

---

Pierwsze pominę, co do wielkości liter, kwestia umowna.

Class było używane przed standardem c++98, w tym standardzie wprowadzono słowo kluczowe typename. By zachować kompatybilność wsteczną oba słowa są wymienne. Moim zdaniem typename bardziej objaśnia, że T jest typem. Reasumując od C++ 17, nie ma znaczenia gdzie, czego użyjesz. Również kwestia umowna. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4659.pdf#page=1445&zoom=100,94,46 Standard sam pozwala na dwa rozwiązania. 

Tak jest! (Czy zamiast cout "stack overflow" powinienem rzucać wyjątkiem?)

Zależy jaką strategię przyjąłeś. 

Apropo new i delete https://dsp.krzaq.cc/post/176/ucze-sie-cxx-kiedy-uzywac-new-i-delete/

---

Pierwsze pominę, co do wielkości liter, kwestia umowna.

Przyjęte jest, że dla nazw klas/struktur stosuje się PascalCase, a dla metod camelCase lub PascalCase .

---

Przyjęte jest, że dla nazw klas/struktur stosuje się PascalCase, a dla metod camelCase lub PascalCase .

Czyli kwestia umowna :>

Poprawiłem - nazwy zaczynają się już duą literą

---

Owszem, ale lepiej się trzymać przyjętych od dawna konwencji. W C++ w zasadzie są dwie szkoły: albo konwencja używana w Javie, czyli PascalCase / camelCase, albo C - funkcje i zmienne z małych liter, makra z dużych.

Answer 1

@niezalogowany,

• dodaj konstruktor kopiujący/przenoszący i takie same operatory.
• samo delete head; nie spowoduje usunięcie wszystkich elementów stosu.
• push powinna przekazywać argument przez const referencję.
• zrób push przyjmującą r-wartość (&&).
• w push wartość stackItem::value ustawiaj w trakcie tworzenia obiektu - po co wywoływać domyślny konstruktor T?

Trzy ostatnie punkty można załatwić prostym zabiegiem:

template <class T>
void stack<T>::push(T value)
{
    head = new stackItem<T>{ std::move(value), head };
}