*** ВНИМАНИЕ: Блог переехал на другой адрес - demin.ws ***

вторник, 26 октября 2010 г.

Никому нельзя верить, даже себе

Есть (точнее был) у меня вот такой код:

const std::string& id() const { return id_; }
std::string id() { return id_; }

В нем есть одна досадная опечатка, из которой код:

order.id() = 123;

делал то, что от него не ожидалось, а точнее, ничего не делал. И проблема, как вы наверное уже догадались, в пропущеном значке "&" во второй строке. Должно было быть так:

const std::string& id() const { return id_; }
std::string& id() { return id_; }

Эта опечатка стоила мне часа поиска проблемы через вторичные признаки в виде иногда не обновляемой базы данных.

Причина? А все потому, что я поленился написать тесты изначально, решив, что это уж очень простые методы. Но теперь таки добавил для этого тест:

TEST(Order, GetterSetters) {
  Order order;
  ...
  EXPECT_EQ(0, order.id());   // Must be initialized.
  order.id() = 123;
  EXPECT_EQ(123, order.id());
  ...
}

Решил сэкономить время, а вышло наоборот.

Вывод: тесты, тесты и тесты.

15 комментариев:

  1. >order.id() = 123;
    Какой некрасивый и неочевидный код, однако.

    Что-то типа такого напоминает:
    5[str] = 'a';

    ОтветитьУдалить
  2. Как насчёт void set_id(const std::string& str) { id_ = str; }
    ?

    ОтветитьУдалить
  3. Как насчет

    public: std::string id_;

    По моему эти методы имеют слишком мало оснований для существования...

    ОтветитьУдалить
  4. @Qehgt: Для С++ это нормальный прием.

    @Vlad: Тут люди пишут поразному. Кто-то любит get/set функции, а кто-то - const/non-const пары. Этот как с перегрузкой операторов в С++. Можно ведь и без них жить, как в Яве. Перегруженные операторы более запутаны на стадии написания и отладки, но их использование нагляднее get/set функций. Так и тут. Лично я, если не ограничен каким-то стандартом кодирования, предпочитаю const/non-const подход, так как нахожу "order.id() = 123;" нагляднее, чем "order.put_id(123);". Идельное решения, конечно, посередине, в Борланде, с их идеей properties класса, когда извне ты пишешь "order.id = 123;" (уже без скобок), а внутри класса это раскрывается в вызов get/set методов. Но есть только в Борланде.

    @Андрей Валяев: Лично я так делаю только если это не класс, а структура, в которой есть только данные. С точки зрения эффектиности при подходе через функции нет никакого проигрыша, зато есть возможноть всега сделать из примитивной функции непримитивную, например, добавить какую-нибудь проверку.

    ОтветитьУдалить
  5. 1) А зечем писать две реализации одного и того же метода с const и без, если они одинаковые? Чтобы показать "какой я крутой, знаю, что методы можно перегружать по признаку константности"? Дублирование кода - зло и проблема тут как раз в том, что два метода в точности повторяют друг друга (с точностью до опечатки).
    2) Писать тесты после того, как был найден и исправлен баг - обычная практика. С чистого листа покрыть код тестами на 100% весьма трудно.

    ОтветитьУдалить
  6. Иметь const/non-const версии не прихоть, а необходимость. Если иметь только const - не можешь модифицировать. Если только non-const, выжден будешь делать все остальное, что этот метод вызывает делать non-const, или снимать const через cast.

    ОтветитьУдалить
  7. Значит баг в дизайне языка.

    ОтветитьУдалить
  8. @Александр - ну почему только в Борланде, VS тоже умеет начиная с 2003-й.
    http://msdn.microsoft.com/en-us/library/yhfk0thd(v=VS.100).aspx

    ОтветитьУдалить
  9. а почему вообще order.id() = 123; скомпилировался? разве есть оператор преобразования целого к строке?

    ОтветитьУдалить
  10. 2Александр: get/set я могу понять... Хотя транспортные структуры вполне могут быть просто структурами...

    А возврат ссылки - это какая-то особая форма эксгибиционизма. То, что это можно делать - вовсе не означает что это правильно. У нас в проектах такое тоже встречается - вот у меня тут мультимап содержащий много всякого - нате его вам, делайте с ним что хотите...

    Необходимости в двух методах тоже не вижу. Константный метод еще можно оправдать, как попытку заоптимизировать передачу результата через голову компилятора...

    Имея два метода ты совершенно не контролируешь константность. ты можешь забыть создать константный контекст и совершенно молча вызовется неконстантный метод...

    ИМХО лучше держаться от таких конструкций подальше, используя либо get/set, либо предоставив прямой доступ к переменной.

    Да и форма записи выглядит жестоко...
    order.id() = 123;

    Сразу начинаешь думать о том что такого может вернуть функция, и просторы для фантазии достаточно широки.

    ОтветитьУдалить
  11. "Идельное решения, конечно, посередине, в Борланде, с их идеей properties класса, когда извне ты пишешь "order.id = 123;"

    Ну почему же? Возможность создавать свойства есть и в фреймфорке QT :) Кстати, очень удобная реализация. Все устанавливается с помощью QT-макроса, в котором указывается имя свойства, геттер и сеттер :)

    ОтветитьУдалить
  12. Методы get/set хорошо контролируют доступ к данным. Зачем открывать данные или выдумывать ещё что-то?

    Использовав вместо set() метод, возвращающий неконстантную ссылку, вы просто на ровном месте создали себе проблему и потратили кучу времени на поиск глупейшей ошибки. С методом set() этого не случилось бы.

    >"order.id() = 123;" нагляднее, чем "order.put_id(123);

    Как же оно может быть нагляднее? Напротив, это очень странный синтаксис: присваивание значения вызову функции. Читатель кода, увидев это, непременно пойдёт смотреть, как работает id(), потому что эта конструкция тупо необычна. В коде с set() никуда ходить не надо - всё и так предельно ясно.

    ОтветитьУдалить