Оцените, пожалуйста, класс для определения курса валют

Приветствую

Решил попробовать свои силы в написании расширения для Yii. Для начала решил написать что-нибудь небольшое, а именно - получение актуальных курсов валют с центрального банка РФ по отношению к рублю с поддержкой кэша.

Скачать можно тут

Буду признателен за отзывы и критику.

Это первый, но надеюсь не последний, OpenSource проект, если можно так сказать, учитывая что в нем 230 строк :) В общем буду рад любым комментариям.

Поправил ссылку.

По опыту написания библиотек для вебсервисов, следующие замечания:

  1. Вместо того, чтобы указывать класс в настройках компонента, лучше сделать по принципу dependency injection - в рамках yii framework, лучше указывать $cacheId, и получать компонент через yii::app()->{$this->cacheId}

  2. throw new Exception - это антипаттерн, т.к. не позволяет понять на уровне кода, что за ошибка произошла. Гораздо лучше сделать свой обьект типа CbrfException, тогда можно уже различать ошибки при помощи блоков try catch.

  3. Непонятно, зачем вручную делать serialize && deserialize - компонент yii ICache сам это делает. Вообще, работа с кэшем как-то слишком сложно организована, можно было бы и легче. Есть простой паттерн для этого - active cache: суть его в том, что если данные из кеша не вернулись, то метод или обьект сам знает, откуда вернуть его. Пример:




if(!($file = Yii::app()->cache->get('cbrf_file')))

{

   $file = file_get_contents($url);

   Yii::app()->cache->set('cbrf_file', $time); // точно не помню сигнатуру

}

return $file;



Что это дает - логика работы с кэшем не размазана по всему компоненту, для конкретного случая весь код расположен в конкретном, единственном методе. И не нужно самому следить за устареванием информации, ICache умеет это делать сам. Когда истечет время жизни кеша, тогда вернутся не данные, а null.

  1. Не обязательно, но разбор xml при помощи регулярок… Я бы использовал любую нативную библиотеку, типа DOMDocument или simplexmllib.

Mitallast, спасибо за ревизию и отзыв

  1. С cacheId согласен, нужно будет подумать как лучше сделать. А класс там прописан не для этого. Он для того, чтобы если в системе нет кэша в настройках, то он создаст свой, используя собственно говоря $cacheClass. Просто чтобы пользователю лишний раз с настройками не копаться, а сразу в бой.

  2. Абсолютно согласен, недосмотрел. А как правильно будет сделать? Наследовать его просто от Exception или от Yii’шного CException?

  3. По поводу serialize, unserialize не знал, гляну, спасибо. По поводу сложности кэша, да есть над чем подумать, но все не так и просто. Дело в том, что время не является показателем устаревания кэша, тут скорее важна дата. Т.е. если кэш создали в 11 вечера 8 августа, то он устареет через час, когда настанет 9 августа, а не через 24 часа. Есть механизм dependency у кэша, но не стал его использовать… хотя можно было бы? Как считаете лучше сделать?

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

  1. simplexml теоретически вроде как может отсутствовать… плюс мне показалось что на таком небольшом массиве данных регулярки будут быстрее, хотя это чисто интуитивное мнение, на самом деле все может быть по-другому.
  1. Тут ситуация, которая частенько возникает в продукции фирмы Apple - я хоть и фанат немного - используется принцип "я сам знаю, что Вам нужно". Я бы не рекомендовал делать настолько суровую защиту от дурака, т.к код теряет свою гибкость и простоту. Лучше оставьте по умолчанию кеш приложения, это лучший вариант.

  2. На мой взгляд, разницы нет. Сейчас я стараюсь писать библиотеки, которые переносимы от фреймворка к фреймворку, и потому посоветую не использовать CException. Лучше обратиться к гаедлайнам популярных разработчиков, например yiiext на google code.

  3. Однозначно, использовать возможности фреймворка. Зависимости в кэше - мощный инструмент! Однако, смысла ставить кэш на целый день не вижу, я бы ставил около получаса. Надеяться на полный аптайм чужих серверов - плохая идея, уж поверьте!

  4. Просто попробуйте оценить, какой вклад в производительность вносит разбор xml в Вашем случае. Результат кэшируется на целый день! А далее, попробуйте оценить, что быстрее и надежнее - выверенный алгоритм разбора xml c линейной практически сложностью, т.к позволяет 100% вероятностью сказать, чем является текущий символ, или регулярка, в которой один и тот же символ может быть просмотрен несколько раз, т.к попадает под некоторую часть вхождения выражения. Simplexml много где есть по дефолту во всяких линуксовых дистрибутивах, и тому подобном. Не считаю это проблемой, т.к вероятность отсутствия не более, чем отсутствия либы регулярок.

Изучайте существующие качественные расширения, типа проекта yiiext, zii и т.п. - в большинстве своем код лаконичен и минимален, каким и должен быть. Не стоит решать проблемы интеграции при помощи навороченных проверок, лучше кидайте exception - информативнее будет. По поводу сложности кэша - уберите для начала его совсем, и минимизируйте свой код, по возможности избавляясь от своих велосипедов, заменяя на проверенные и понятные решения, типа libsimplexml. В идеале, должен остаться тот код, который сам по себе является документацией - для этого он должен быть лишен воды, быть максимально точным и однозначным!

Я тоже глянул из интереса в исходный код и за кучей методов, занимающихся кэшированием, не заметил “главного” :) И это учитывая, что кэширования Yii должно хватать “за глаза”. Ну и SimpleXML, конечно, здесь более уместен, чем регулярки. Поправьте эти моменты, и будет отличный компонент ;)

я тоже за SimpleXML

запросы на XPath выглядят куда более лаконично и понятно, чем регулярки

И еще раз всем спасибо за отзывы и пожелания.

Что сделано:

  1. Добавлены Exception’ы причем даже 2 вида :)

  2. Решено отказаться от кэша по времени и перейти к кэшу по дате. Для чего написан дополнительный класс CbrfDateDependency

  3. Добавлена поддержка simplexml

  4. Добавлена защита от задержек по времени (кэш будет сброшен) и ошибок ответа сервера (будут взяты предыдущие значения).

Так и сделано, причем уже в первой версии. По умолчанию берется кэш из Yii::app()->{cahceId} если он не указан, то создается локальная версия кэша

Решил пока наследовать от CException, все равно расширение рассчитано на Yii.

Согласен. Решил полностью отказаться от времени с привязкой к дате и защитой от некорректных данных.

Пока конечно код не идеален, но еще подумаю как грамотно сделать рефакторинг.

В общем буду рад новым отзывам, всем спасибо!

Новая версия где и была https://github.com/dlitsman/YiiCbrf/blob/master/Cbrf.php

Еще бы информацию по металлам класс получал, цены бы ему не было ;)

Подумаю над этим :) но пока хотелось бы до ума довести курсы валют :)

Форкнул и порефакторил немножко =)

Я его не тестил, хочу просто показать, что с кэшем нет нужды так сильно мутить.

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

  • как я говорил, кэш теряет свою актуальность раз в сутки, а не раз в 24 часа, для чего добавлен CbrfDateDependency. Разве это не важно? Просто может я ошибаюсь…

Пока не все поглядел, но идеи с loadXml() и getCache() понравилась.

В Вашем коде, насколько помню, просто сравнивалась дата в кэше и не в кэше. Это я и реализовал через вычисление expire time. Иначе поясните, что имели в виду? Понятно, что новый день != 24 часа, это я как раз учел.

Нет у Вас никакой защиты от ошибок на сервисе - кэш не является persistent хранилищем, надеяться на 100% постоянную работу бессмысленно - возьмите например memcache, у которого кэш сам по себе очищается по мере заполнения данными, и очищение может произойти раньше expire time. Правильный путь - использование persistent хранилищ - от базы данных, до файла на жестком диске.

Действительно, не заметил. Отличное решение, жаль до него не догадался, спасибо! Наверно откажусь от своего в пользу него.

По второму пункту тоже вынужден согласится, наверно сделаю файл с курсами валют в папке расширения. Так будет нормально? Базу все таки прикручивать довольно проблематично, да и не нужна она тут.

Чуть позже тогда выкачу еще 1 версию на суд общественности :)

Спасибо за конструктивную критику!

Всегда пожалуйста!

Файл - нормальный способ! Используйте json, сохраняйте по умолчанию в директорию app.runtime (не помню как она там называется), но давайте возможность настроить свой путь.

Насчет решения - попробуйте думать не как программист, а как математик. В большинстве случаев, достаточно написать некий алгоритм, представив его в виде формулы.

Меня интересует один вопрос. Я правильно понимаю, что кому-то из посетителей сайта придется “подождать”, когда будет обновляться кэш? Запрос к cbr.ru может выполняться несколько секунд, никто не гарантирует, что чужой сервер ответит быстро. Конечно, это совсем не критично, но, возможно, такие задачи лучше запускать через cron? Кэширование сложных запросов к собственной базе данных это другое дело :)

И да, я жду курсов по металлам, не думайте, что это никому не нужно ;) Только не говорите “лучше бы взял и сам сделал, заодно помог бы”. Сейчас очень занят, да и мне не к спеху :lol:

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

Да, правильно. По-другому делать только кроном… но сделать крон в принципе не проблема. Т.к. sourceUrl можно поменять, то можно поставить скрипт на крон который будет раз в день/час/минуту скачивать информацию с cbr.ru и писать ее в файл. А расширение будет брать информацию уже из файла… тогда задержки точно не будет для конечного пользователя. Или просто самому по крону исполнять расширение, тогда в теории может быть небольшая задержка для кого-то в районе 12, если он успеет раньше крона обратиться… но думаю это не критично.

Согласен подумаю, наверно сделаю :)

Ок, если силы и настрой останутся :) Но опять же, после доведенных до ума курсов. Просто курсы мне и самому скоро понадобятся, а металлы пока нет.

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

Да, там есть тонкости. Например, на выходных и в праздничные дни курсы не обновляются, и нужно показывать не сегодняшнюю дату, а дату пятницы (в случае выходных). Соответственно, нам нужны последний курс среди предыдущих дней и курс завтрашнего дня. Последний может отсутствовать.