Замечания по StringList

[alexey-malov]

    CStringList(CStringList & list);

• [x] Это не конструктор копирования. Следующий код либо не скомпилируется, либо приведет к падению:

CStringList DoSomething(const CStringList & lst)
{
    return lst;
}

[alexey-malov]

    void CStringList::Insert(const CListIterator<std::string> & it, std::string data);
    void CStringList::Erase(const CListIterator<std::string> & it);

• [x] внутри класса указывать имя текущего класса не нужно

[alexey-malov]

    std::string & GetLastElement() const;
    std::string & GetFirstElement() const;

• [x] Пользователь может изменить первый и последний элемент константного списка, что ломает всю идею константности

    const std::string & GetLastElement() const;
    const std::string & GetFirstElement() const;

• [x] Но теперь пользователь не может изменить первый и последний элементы неконстантного списка

[alexey-malov]

    const CListIterator<std::string> cbegin() const;
    const CListIterator<std::string> cend() const;

• [x] cbegin и cend должны возвращать readonly-итератор, позволяющий перебирать значения но не модифицировать их

• [x] Аналогично crbegin и crend

[alexey-malov]

    CListIterator<std::string> begin();
    CListIterator<std::string> end();

• [x] Для begin и end должны быть также const-версии, ведущие себя аналогично cbegin и cend

• [x] Аналогично для rbegin и rend

[alexey-malov]

• [x] Реверсированный и нереверсированный итератор должны быть разными типами, иначе можно присвоить переменной, хранящей реверсированный итератор, переменную, хранящую нереверсированный итератор и поведение объекта сразу поменяется. Смотрите в сторону std::reverse_iterator

[alexey-malov]

• [x] Не реализован оператор присваивания.

• [x] Не реализованы конструктор и оператор перемещения

CStringList& CStringList::operator=(CStringList const& rhs)
{
    Copy(rhs);
    return *this;
}
void CStringList::Copy(CStringList const& rhs)
{
    Init();
    for (const std::string & elem : rhs)
    {
        PushBack(elem);
    }
}

• [ ] Проблема с самоприсваиванием

• [ ] Если Init выбросит исключение, то объект останется в невалидном состоянии.

• [ ] При вызове Init может возникнуть переполнение стека из-за глубокой рекурсии при разрушении длинного списка узловю

• [ ] Оператор копирующего присваивания не обеспечивает строгую гарантию безопасности исключений, а только базовую

CStringList& CStringList::operator=(CStringList && rhs)
{
    Move(std::move(rhs));
    return *this;
}

void CStringList::Move(CStringList && rhs)
{
    m_firstNode = std::move(rhs.m_firstNode);
    m_lastNode = rhs.m_lastNode;
    rhs.m_lastNode = nullptr;
    m_size = rhs.m_size;
    rhs.m_size = 0;
    rhs.Init();
}

• [ ] Проблема с самоперемещением

• [ ] Если rhs.Init выбросит исключение, то rhs останется в невалидном состоянии (без сторожевых узлов либо с неполным их составом)

[alexey-malov]

void CStringList::PushBack(const std::string & data)
{
    try
    {
        std::unique_ptr<ListNode> newNode = std::make_unique<ListNode>(data, m_lastNode, nullptr);
        ListNode *newLastNode = newNode.get();
        if (m_lastNode)
        {
            m_lastNode->next = std::move(newNode);
        }
        else
        {
            m_firstNode = std::move(newNode);
        }
        m_lastNode = newLastNode;
        m_lastNode->next = nullptr;

        ++m_size;
    }
    catch (...)
    {
        throw;
    }
}

• [ ] Ловить исключение и сразу его перевыбрасывать бессмысленно

• [ ] Обнулять m_lastNode->next излишне

[alexey-malov]

void CStringList::PushFront(const std::string & data)
{
    try
    {
        std::unique_ptr<ListNode> newNode = InsertOnTheEdge(data, nullptr, std::move(m_firstNode));
        if (!newNode->next)
        {
            m_lastNode = newNode.get();
        }
        m_firstNode = std::move(newNode);
        m_firstNode->prev = nullptr;

        m_size++;
    }
    catch (...)
    {
        throw;
    }
}

• [ ] Обнулять m_firstNode->prev излишне

• [ ] Ловить и сразу перевыбрасывать исключение бессмысленно

[alexey-malov]

• [ ] Как декрементировать итератор, указывающий на end?

[alexey-malov]

CListIterator<std::string> CStringList::end()
{
    return (m_lastNode != nullptr) 
        ? CListIterator<std::string>(m_lastNode->next.get(), false) 
        : CListIterator<std::string>(m_lastNode, false);
}

• [ ] Обе ветки тернарного оператора возвращают итератор, указывающий на null

[alexey-malov]

private:
    ListNode* m_node = nullptr;
    bool m_isReverse = false;

    ListNode* operator->() const
    {
        return m_node;
    }

• [ ] Оператор -> должен быть публичным и возвращать указатель на string (на данные, а не на узел)

[alexey-malov]

• [ ] Не реализованы постфиксные инкремент и декремент итератора

[alexey-malov]

    bool operator==(CListIterator const& rhs) const
    {
        return m_node == rhs.m_node;
    }

    bool operator!=(CListIterator const& rhs) const
    {
        return m_node != rhs.m_node;
    }

• [ ] Итераторы считаются равными, даже если их флаг реверсированности различается. Этого не должно быть.

[alexey-malov]

void CStringList::Insert(const CListIterator<std::string> & it, std::string data)
{
    if (it == begin())
    {
        PushFront(data);
    }
    else if (it == end())
    {
        PushBack(data);
    }
    else
    {
        try
        {
            auto node = std::make_unique<ListNode>(data, it->prev, std::move(it->prev->next));
            it->prev = std::move(node.get());
            node->prev->next = std::move(node);
        }
        catch (...)
        {
            throw;
        }
    }
}

• [ ] move для примитивных типов выполняет копирование. move(node.get()) просто копирует, а не перемещает. Лучше не вводить в заблуждение.
• [ ] Размер списка при вставке в середину не модифицируется.

[alexey-malov]

• [ ] После перемещения над списком-донором можно выполнить разве что только операцию разрушения. Пользоваться им нельзя, т.к. из него удаляются граничные узлы. Требование такое: объектом-донором должно быть возможно пользоваться.

[alexey-malov]

void CStringList::Init()
{
    m_firstNode = std::make_unique<ListNode>("", nullptr, nullptr);
    m_lastNode = new ListNode("", nullptr, nullptr);
    m_firstNode->next = std::unique_ptr<ListNode>(m_lastNode);
    m_lastNode->prev = m_firstNode.get();
    m_size = 0;
}

• [ ] конечную ноду тоже создать через make_uniqu

[alexey-malov]

void CStringList::PushBack(const std::string & data)
{
    try
    {
        std::unique_ptr<ListNode> newNode = std::make_unique<ListNode>(data, m_lastNode->prev, std::move(m_lastNode->prev->next));
        ListNode *newNodePtr = newNode.get();
        m_lastNode->prev->next = std::move(newNode);
        m_lastNode->prev = newNodePtr;

        ++m_size;
    }
    catch (...)
    {
        throw std::runtime_error("Insertion failed");
    }
}

• [ ] Убрать try-catch

[alexey-malov]

• [ ] реверсивный итератор и обычный должны быть разными типами

[alexey-malov]

class CStringList
{
    friend class ListNode;

• [ ] Зачем CStringList объявлять своим другом ListNode?

[alexey-malov]

• [ ] Зачем в CStringList публичный метод Init?

[alexey-malov]

• [ ] Код кривой, сделать по-человечески

[alexey-malov]

• [ ] У итератора должен быть оператор ->, возвращающий указатель на данные

[alexey-malov]

void CStringList::Clear()
{
    while (m_lastNode)
    {
        m_lastNode->next = nullptr;
        m_lastNode = m_lastNode->prev;
    }
    Init();
}

• [ ] Если Init выбросит исключение, объект останется в невалидном состоянии (отсутствующие крайние узлы)

• [ ] Т.к. Clear вызывается из деструктора, то есть риск выбрасывания исключения деструктором.