Замечания по программе Calculator

[alexey-malov]

• [ ] Доработать парсер пользовательского ввода, чтобы можно было пропускать пробелы, где это возможно

[alexey-malov]

• [x] Программа позволяет переопределять значения функций, что даёт возможность циклических ссылок. Запретить. Покрыть тестами

[alexey-malov]

• [x] Обмен данными с калькулятором производить не через строки, а с использованием чисел. Полезным может оказаться тип std::optional для возврата значений, неинициализированных переменных.

• [x] Переменные типа double могут хранить значение NAN http://www.cplusplus.com/reference/cmath/isnan/ http://www.cplusplus.com/reference/cmath/nan-function/

[alexey-malov]

• [x] В Calculator использовать для задания операции, выполняемой в функции не строку, а enum.

Преобразование из/в строковое представление лучше оставить в CalcControl

[alexey-malov]

• [x] Multimap предназначен для хранения дублирующихся ключей. В задании можно обойтись при помощи map

[alexey-malov]

• [x] Убрать у функции возможность её переопределения

[alexey-malov]

• [x] В IValueRetriever можно вместо Have* (лучше подходит Has) использовать метод TryGetValue, возвращающий optional, либо просто double (со значение NaN для неинициализированных значений).

[alexey-malov]

class CCalculator
{
public:
    CCalculator() = default;
    ~CCalculator() = default; 

• [x] Компилятор сам может сгенерировать деструктор и конструктор по умолчанию (в отсутствие других конструкторов, заданных пользователем)

[alexey-malov]

• [x] Методы, не меняющие состояние калькулятора должны быть константными. Для сигнализирования об ошибке можно использовать исключения, либо возвращать ошибку через возращаемое значение или out-параметр

[alexey-malov]

• [ ] Лучше ввести два конструктора в CFunction - один для унарной функции, а другой для бинарной

[alexey-malov]

• [ ] Написать тесты для CalcControl

[alexey-malov]

• [ ] Добавить поддержку унарных функций в приложение

[alexey-malov]

bool CCalculator::SetVar(const std::string& identifier, std::string& errorMsg)
{
    if (!CanBeIdentifier(identifier))
    {
        errorMsg = "identifier name incorrect";
        return false;
    }

    auto it = m_names.find(identifier);
    if (it == m_names.end())
    {
        auto val = std::make_shared<CVariable>();
        auto var = std::make_pair(identifier, val);
        m_names.insert(var); // (1)
        m_vars.insert(var); // (2)
        return true;
    }

    errorMsg = "identifier already exist";
    return false;
}

• [ ] Если в (2) выбросится исключение, то надо удалить элемент, добавленный в (1). Иначе будет несогласованное состояние калькулятора

[alexey-malov]

bool CCalculator::SetValue(const std::string& identifier, const double value, std::string& errorMsg)
{
    if (!CanBeIdentifier(identifier))
    {
        errorMsg = "identifier name incorrect";
        return false;
    }

    std::pair<std::string, std::shared_ptr<CVariable>> var;

    auto it = m_vars.find(identifier);
    if (it != m_vars.end())
    {
        var = *it;
    }
    else
    {
        var = std::make_pair(identifier, std::make_shared<CVariable>());
        m_names.insert(var);
        m_vars.insert(var);
    }

    var.second->SetValue(value);
    UpdateFns();
    return true;
}

• [ ] Аналогичное замечание.
• [ ] Также лучше добавить noexcept в сигнатуру методов и функций, которые не бросают исключений

[alexey-malov]

• [ ] SetFn() аналогичное замечание насчёт безопасности исключений

[alexey-malov]

    auto it_var = m_names.find(firstOperand);

• [ ] it_var - не используйте snake_case

[alexey-malov]

• [ ] При помощи emplace можно сразу вставлять значения в map, не создавая промежуточный объект

[alexey-malov]

• [ ] m_fns_update_list не использовать snake_case