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

[alexey-malov]

TEST_CASE("Testing channel switching")
{
    CTVSet tv;
    tv.TurnOn();

    CHECK(tv.IsTurnedOn() == true);
    CHECK(tv.GetChannel() == 1);

    CHECK(tv.SelectChannel(0) == false);
    CHECK(tv.SelectChannel(101) == false);
    CHECK(tv.SelectChannel(50) == true);
    CHECK(tv.GetChannel() == 50);

    tv.SelectChannel(2);
    CHECK(tv.SelectPreviousChannel() == true);
    CHECK(tv.GetChannel() == 50);
}

• [x] Тесты должны не только проверять успешность или неудачу при выполнении модифицирующей состояние объекта операции, но еще и сравнивать состояние объекта после операции с ожидаемым. Если в разных тестах используются однотипные проверки состояния (состоящие из нескольких действий), то полезным будет выделить их в функции вида:

  
  void ExpectOperationSuccess(CTVSet& tv, const function<bool(CTVSet&tv)>& op, const optional<int>& expectedChannel)
  {
  CHECK(op(tv)); // убедились, что операция завершится успешно
  // сравниваем включенность и канал с ожидаемыми значениями
  CHECK(tv.IsTurnedOn() == expectedChannel.has_value());
  CHECK(tv.GetChannel() == expectedChannel.value_or(0));
  }

void ExpectOperationFailure(const CTVSet& tv, const function<bool(CTVSet&tv)>& op) { CTVSet tvCopy(tv); // копия телевизора, над которой будет выполняться операция CHECK(!op(tvCopy)); // убедились, что операция (над копией) завершится неуспешно // сравниваем включенность и канал с прежними значениями CHECK(tvCopy.IsTurnedOn() == tv.IsTurnedOn()); CHECK(tvCopy.GetChannel() == tv.GetChannel()); }

TEST_CASE("TV can select channels from 1 to 99 only when it is on") { CTVSet tv; tv.TurnOn();

ExpectOperationSuccess(tv, [](CTVSet& tv){ return tv.SelectChannel(99); }, 99);
ExpectOperationFailure(tv, [](CTVSet& tv){ return tv.SelectChannel(100); });
...

}

[alexey-malov]

• [x] Сделать тесты более ориентированными на проверку отдельных сценариев поведения

[alexey-malov]

• [x] Добавьте пустые строки в объявление класса CTVSet, методы сливаются

[alexey-malov]

• [x] Если вы ввели структуру ChannelInfo, то пусть метод CTvSet::GetChannelList возвращает ее а не pair.

• [x] Тип pair плохо подходит для интерфейсных вещей. Лучше сделать структуру с полями number и name, чем first и second

[alexey-malov]

bool CTVSet::SetChannelName(int channel, string const& name)
{
    bool isAvailableChannel = (channel >= 1) && (channel <= 99);
    if (!isAvailableChannel || !m_isOn)
    {
        return false;
    }
    if (CTVSet::GetChannelByName(name) != 0)

• [x] Не нужно CTVSet:: писать в данном случае

[alexey-malov]

• [x] Посмотрите в сторону контейнера boost::bimap (bi-directional map). Там можно эффективно искать как по первому, так и по второму значению. Возможно, подойдёт лучше чем вектор

[alexey-malov]

    for (auto currChannel= m_channelInfo.begin(); currChannel != m_channelInfo.end(); currChannel++)
    {
        if (currChannel->first == channel)
        {
            currChannel->second = name;
            return true;
        }
    }

• [x] Если в векторе хранятся пары номер-имя для всех каналов, то последовательный поиск можно заменить на обращение по индексу.

[alexey-malov]

• [x] Циклы поиска канала по имени следовало бы выделить в отдельную функцию.

[alexey-malov]

> Info
TV is turned on
Channel is: 1
4 - TV 3
9 - 1st
> SelectChannel 1st
TV switched on 1 channel.

• [ ] Если номер канала начинается с цифры, то RemoteControl ошибочно воспринимает его в виде числа.

• [ ] В тестах RemoteControl надо проверить эту ситуацию

[alexey-malov]

function<bool(CTVSet& tv)> SelectChannel(int channel)
{
    return [channel](CTVSet& tv) {
        return tv.SelectChannel(channel);
    };
}

function<bool(CTVSet& tv)> SetChannelName(int channel, const string& name)
{
    return [channel, name](CTVSet& tv) {
        return tv.SetChannelName(channel, name); 
    };
}
...
ExpectOperationSuccess(tv, SelectChannel(99), 99);

[alexey-malov]

SCENARIO("deleting channel")
{
    GIVEN("A turned of TV")
    {
        CTVSet tv;
        WHEN("TV is turned on")
        {
            tv.TurnOn();
            THEN("if the channel is named, you can delete the name")
            {
                tv.SetChannelName(1, "ORT");
                CHECK(tv.GetChannel() == 1);
                tv.DeleteChannelName("ORT");
                CHECK(tv.GetChannelName(1) == "");
            }
            AND_THEN("if the channel name is not assigned the status will not change")
            {
                ExpectOperationFailure(tv, [](CTVSet& tv) {return tv.DeleteChannelName("TNT"); });

• [ ] Не обязательно начинать сценарий с чистого листа (с выключенного/включенного телевизора). Здесь можно начать с телевизора, у которого уже задана ассоциация названия с номером канала

SCENARIO("Channel deletion")
{
    GIVEN("A turned on TV with associated channel name")
    {
        CTVSet tv;
        tv.TurnOn();
        tv.SetChannelName(3, "TV 3");
        WHEN("deleting the associated channel")
        {
            CHECK(tv.DeleteChannelName("TV 3"));
            THEN("the deleted channel can not be selected")
            {
                ExpectOperationFailure(tv, SelectChannel("TV 3"));
            }
        }
    }
}

[alexey-malov]

int CTVSet::ChannelNumberByName(string const& name, vector<channelInfo> const& channelInfo)
{
    for (auto currChannel = channelInfo.begin(); currChannel != channelInfo.end(); currChannel++)
    {
        if (currChannel->channelName == name)
        {
            return currChannel->channel;
        }
    }
    return 0;
}

• [ ] методы класса не меняющие его состояние, должны быть константными

[alexey-malov]

• [ ] Некоторые ветки классов CTVSet и CRemoteControl не покрыты тестами. При помощи OpenCPPCoverage (описание в группе есть) найдите их и напишите недостающие тесты

[alexey-malov]

• [ ] Метод CRemoteControl::isValidChannel должен начинаться с заглавной буквы