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

eligantRU commented 5 years ago

[x] Где main.cpp?
[x] Комментарии
[x] Самообман
```
bool isHandlerOn = 1; // bool != int
```

[x] КПД достиг нуля

    stringstream sstream(command);
    sstream >> command;

[x] Не следует создавать неинициализированные переменные
[x] Команды можно вынести в Enum
[x] UpperCamelCase
[x] Избыточные пустые строки
[x] Тесты располагаются в отдельном проекте того же решения
[x] Фигурные скобки ставятся с новой строки, исключение - лямбды
[x] Проверить строку на число можно проще - существует функция ::isdigit, которая проверяет char на 0..9, в паре с all_of - сочетается неплохо
[x] Константы характерные для класса либо знает сам класс и все его использующие, либо только сам класс, глобальные(и не очень) константы пишутся как-то так: MAX_CHANNEL
[x] Поля класса имеют префикс m_
[x] Может ли быть отрицательным номер канала?
[x] Использовать механизм исключений
[x] Константные переменные могут быть объявлены как const
[x] Shit-in - shit-out - не лучший подход
[x] Семейство алгоритмов STL располагает std::find_if
[x] boost::trim избавит от написания велосипедов(которые тоже нужно обкладывать тестами)
[x] Если что-то можно передать по константной ссылке, то почему бы и да?
[x] Если хочешь получить номер канала - добавь accessor(getter), хочешь изменить - mutator(setter)
[x] Все assert'ы в Release-конфигурации удаляются из кода
[x] У строк есть метод empty()
[x] У строк есть метод size()
[x] INT_MAX? std::numeric_limits
[x] Boost.Test

eligantRU commented 5 years ago

(1.1 0.67 + 1.0 0.08) 50 + 1.0 10 + (1.0 0.75 + 0.8 0.2) * 40

eligantRU commented 5 years ago

[x] Код не компилируется
[x] Проект невалиден

[x] Почему и не &name?

auto it = std::find_if(m_channels.begin(), m_channels.end(), [name](Channel channel) {
    return (channel.GetName() == name);
});

[x] if (!m_stateOn )
[x] ValidateName - звучит как что-то безобидное, а не изменяющее аргумент
[x] ConsoleCommands - поля Enum'а тоже придерживаются UpperCamelCase'а
[x] string commandToString(ConsoleCommands index) - index?
[ ] string commandToString(ConsoleCommands index) - unexpected command?
[ ] Возможно, разумнее приводить команду к Enum'у, а не наоборот
[x] Просится в функцию

    cout << "Turn on tv - 'TurnOn'" << endl;
    cout << "Turn off tv - 'TurnOff'" << endl;
    cout << "Select channel by number - 'SelectChannel <number>'" << endl;
    cout << "Select channel by string - 'SelectChannel <string>'" << endl;
    cout << "Select previous channel - 'SelectPreviousChannel'" << endl;
    cout << "Set channel name - 'SetChannelName <channel number> <new name>'" << endl;
    cout << "Delete channel name - 'DeleteChannelName <name for delete>'" << endl;
    cout << "Get tv info - 'Info'" << endl;
    cout << "Exit from program - 'Exit'" << endl;

[x] isHandlerOn - "нужно продолжать?"
[x] catch (const std::exception& err) -> catch (const exception & ex)
[x] Channel::IsNameEmpty() - HasEmptyName
[x] m_channels - vector? array? map? Boost.??? ?
[x] Переменная с именем number говорит о себе слишком мало

[x] Можно проще:

throw std::invalid_argument("Channel out of range 1.." + std::to_string(MAX_CHANNEL_NUMBER - 1));

[x] m_stateOn -> m_isTurnedOn
[x] throw std::invalid_argument("String can't be empty"); - исключение мало о чём говорит
[x] ranged-based for умеет в auto
[ ] std::optional может сильно упростить работу с именами каналов UPD: Теперь канал явным образом может стать невалиден при присваивании пустой строки в имя
[x] (number <= 0 && number >= MAX_CHANNEL_NUMBER) - вынести в функцию
[x] Лямбды умеют в auto
[x] Что говорит задание про предыдущий канал, когда телевизор только-только включили?

eligantRU commented 5 years ago

[x] std::swap

        const unsigned temp = m_currChannel;
        m_currChannel = m_prevChannel;
        m_prevChannel = temp;

eligantRU commented 5 years ago

[ ] Тесты не запускаются после сборки
[x] Тесты не пишутся через ктрл+с + ктрл+в - относительные пути исправят проблему
[x] /W4
[x] BOOST_REQUIRE_THROW/BOOST_CHECK_THROW
[x] Нет проверок на переключение канала у выключенного телевизора
[x] currNullName - имя текущего канала-ничтожества
[ ] OpenCppCoverage - для проверок полноты тестов
[x] BOOST_FIXTURE_TEST_SUITE

ImbaCow / OOP

Замечания по TVSet #5