alexanderBaranov / OOP

OOP
0 stars 0 forks source link

Замечание по программе CalculationByRules #16

Open alexey-malov opened 8 years ago

alexey-malov commented 8 years ago

Каждый раз переводить из строкового представления в числовое - неэффективно. Воспользуйся значением NAN для хранения неинициализированных значений. При этом в результате операций сложения и вычитания, значение NAN одного или двух операндов даст в результате также NAN

alexey-malov commented 8 years ago

сортировка значений не нужна, если хранить в map-е пары ключ-значение. Ключи будут отсортированы

alexey-malov commented 8 years ago

можно воспользоваться функцией isnan для того, чтобы определить, является ли значение Не-числом

alexey-malov commented 8 years ago

CalculatFunction -> CalculateFunction

alexey-malov commented 8 years ago
void CalculatFunction(const vector<string>& quote, Values& draft, Values& results)

Функция называется "Вычислить функцию", а ее аргументы не понятно какой несут смысл. Есть конфликт между названием функции и тем, что она делает

alexey-malov commented 8 years ago
void CalculatFunction(const vector<string>& quote, Values& draft, Values& results)
{
    const string& functionName = quote[1];
    const string& operation = quote[2];
    const string& param1 = quote[3];
    const string& param2 = quote[4];

Нет проверки на количество параметров в quote. Надо либо функции передать параметры уже распарсенные, либо проверку на количество аргументов запилить, либо использовать метод at вместо [], который будет бросать исключение при выходе за пределы массива

alexey-malov commented 8 years ago
    if ((draft[param1] == NONE) || (draft[param2] == NONE) || draft[param1].empty() || draft[param2].empty())

В ходе вычисления выражения if в draft могут неявно положиться пустые строки в качестве значений ключей param1 и param2, тем самым добавляя немного мусора в данные, которыми оперирует программа. Этот мусор здесь может быть отфильтрован за счет доп. проверок на пустоту значений, но выглядит это как костыли. При этом ты выполняешь поиск повторно. Лучше не допускать появление мусора в данных, тогда не придется его отфильтровывать (что можно будет забыть при дальнейшем развитии программы).

alexey-malov commented 8 years ago
        double numberOfParam1 = stod(draft[param1]);
        double numberOfParam2 = stod(draft[param2]);
        if (operation == PLUS)
        {
            numberOfParam1 += numberOfParam2;
        }
        else if (operation == MINUS)
        {
            numberOfParam1 -= numberOfParam2;
        }

        draft[functionName] = to_string(numberOfParam1);

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

alexey-malov commented 8 years ago
string GetValueForParam(const string& param, Values& values)
{
    string value = values[param];
    return !value.empty() ? value : NONE;
}

В функцию, которая. по идее, не должна менять коллекцию значений, следует передавать эту коллекцию по константной ссылке. В map есть метод find, который возвращает итератор на элемент с заданным ключом, а также метод at, который возвращает ссылку на значение по ключу, либо бросает исключение в случае отсутствия ключа.

alexey-malov commented 8 years ago

Входные данные программы должны поступать из stdin и записываться в stdout Можно использовать перенаправление ввода и вывода

alexey-malov commented 8 years ago

В выводимых данных пробел между идентификатором и двоеточием стоять не должен (если вы не французы)

alexey-malov commented 8 years ago

с istream, связанным со стандартным потоком, seek работать не будет данные должны считываться за один проход. объем данных, поступающих из stream таков, что в оперативную память может не влезть. В памяти следует хранить только модель данных.

alexey-malov commented 8 years ago

alexey-malov commented 8 years ago
        if (!quote.empty())
        {
            if (quote[2] == PARAM)
            {
                draft[quote[1]] = GetValueForParam(quote[1], values);
            }
            else 
            {
                auto pairResult = CalculateFunctionFromRuleForValuesFromDraft(quote, draft);
                if (!pairResult.first.empty())
                {
                    results[pairResult.first] = pairResult.second;
                }
            }
        }

Проверка идет на empty, а доступ осуществляется к 1, 2 элементам.

alexey-malov commented 8 years ago
    auto iteratorOfParam1 = draft.find(param1);
    auto iteratorOfParam2 = draft.find(param2);
    if (isnan(iteratorOfParam1->second) || isnan(iteratorOfParam2->second))
    {
        draft[functionName] = NAN;
    }

Итератор нельзя разыменовывывать, не проверив, что он ссылается на существующий элемент контейнера (не равен container.end())

alexey-malov commented 8 years ago
        double resultOfOperation;
        if (operation == PLUS)
        {
            resultOfOperation = iteratorOfParam1->second + iteratorOfParam2->second;
        }
        else if (operation == MINUS)
        {
            resultOfOperation = iteratorOfParam1->second - iteratorOfParam2->second;
        }

в случае, когда операция - ни плюс и ни минус, то желательно воткнуть assert или throw

alexey-malov commented 8 years ago

выпилить пробел перед двоеточием

alexey-malov commented 8 years ago

k=0,8