Замечания по стеку строк

[alexey-malov]

class CStringStack
{
public:
    CStringStack();
    ~CStringStack();

    void push(const std::string& value);
    void pop();
    std::string get() const;
    bool isEmpty() const;
private:

    std::unique_ptr<StackElement> last_data;
};

• [x] Нарушены соглашения о стиле кодирования. Имена методов должны использовать UpperCamelCase, а полей класса - m_lowerCamelCase

• [x] что за имя last_data. лучше назвать m_top

[alexey-malov]

• [x] Если в стеке будет порядка 10000 - 100000 элементов, при его разрушении у вас произойдет переполнение стека компьютера из-за глубины рекурсии при вызове деструкторов элементов. Решить эту проблему. Написать тест, проверяющий эту ситуацию и убедиться, что он до исправления не проходит, а после исправления - проходит

    BOOST_AUTO_TEST_CASE(testing_for_many_push_and_pop)
    {
        for (size_t i = 0; i < 100000; i++)
        {
            stringStack.Push("qwerty");
        }
        BOOST_CHECK(stringStack.IsEmpty() != true);
        for (size_t i = 0; i < 100000; i++)
        {
            stringStack.Pop();
        }
        BOOST_CHECK(stringStack.IsEmpty() == true);
    }

• [ ] В тесте убедиться, что при разрушении стека большого размера программа продолжила работать

[alexey-malov]

• [x] Вместо NULL используйте nullptr

[alexey-malov]

        last_data = std::unique_ptr<StackElement>(new StackElement());

• [x] для конструирования используйте make_unique

[alexey-malov]

• [x] можно обойтись без if. Исправить

void CStringStack::push(const std::string & value)
{
    if (last_data == NULL)
    {
        last_data = std::unique_ptr<StackElement>(new StackElement());
        last_data->string = value;
        last_data->ptr = NULL;
    }
    else
    {
        std::unique_ptr<StackElement> p(new StackElement());
        p->ptr = std::move(last_data);
        p->string = value; // если выброчится исключение, то стек окажется пустым
        last_data = std::move(p);
    }   
}

• [x] если при присваивании value выбросится исключение, то last_data окажется обнуленным (т.к. указатель на вершину будет перенесен в p->ptr, который после выхода из области видимости обнулится, удаляя за собой все данные (с возможным переполнением стека из-за рекурсии при деструкторах)

[alexey-malov]

void CStringStack::pop()
{
    if (!isEmpty())
    {
        auto p = std::make_unique<StackElement>;
        last_data = std::move(last_data->ptr);
    }
    else
    {
        throw std::exception("Stack is empty");
    }

}

• [x] p адрес функции make_unique. Зачем?

[alexey-malov]

• [x] сделать отступы в тестах

[alexey-malov]

• [x] Имена и структура тестов должны играть роль спецификации на тестируемую функцию или класс, а не просто перечислить ситуации, в которых вы проводите испытание класса или функции.
• [x] тесты покрывают лишь малую часть поведения класса

[alexey-malov]

• [ ] Устранить проблему с постоянной пересборкой программы. Включите детальный режим диагностики в настройках Visual Studio Build and Run, там выведет из-за нахватки какого файла происходит пересборка

[alexey-malov]

• [ ] У std::exception есть только default constructor и copy constructor. Не создавайте std::exception явно, создавайте его наследников, подходящих по смыслу