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

Эволюция кода

Как получает так, что в целом неплохой код со временем становится помойкой, клубком проблем, Авгиевыми конюшнями и ящиком Пандоры all-in-one?

The Broken Window Theory

A window gets broken at an apartment building, but no one fixes it. It's left broken. Then something else gets broken. Maybe it's an accident, maybe not, but it isn't fixed either. Graffiti starts to appear. More and more damage accumulates. Very quickly you get an exponential ramp. The whole building decays. Tenants move out. Crime moves in. And you've lost the game. It's all over.

Аналогично и с программированием. Вы написали код, хороший код, почти идеальный. От "почти" до идеала вас отделила какая то мелочь. То ли не вовремя закончился рабочий день, а с ним и вдохновение, то ли не кстати появилась критическая проблема, которую срочно нужно было решать. В конце концов мы не идеалисты, мы реалисты! И, черт побери, этот код работает, что еще нужно?!

Прошло время и оказалось что кому то в вашем идеальном коде не хватило той самой мелочи, например точки расширения. Будучи то ли слишком скромным, то ли слишком самостоятельным, этот кто то отвлекать вас не стал, а сам добавил в один из ваших базовых классов какой то метод для своих целей. Прошло еще время и этот метод стал использоваться во многих местах вашего и не только вашего кода. Заодно этот метод обзавелся "соседями" - другими методами на всякие другие случаи жизни. Ваш код заматерел, оброс методами-"мясом", однако оригинальная цель и идея уже не так явно просматривается в этом наборе методов "на-все-случаи-жизни". Зато у многих ваших коллег есть четкое понимание куда нужно добавлять новые методы-помощники, если вдруг такие понадобятся. Взамен вы лишились возможности навести в, когда то вашем, коде порядок, потому что теперь вы связаны по рукам и ногам совместимостью, связанностью и т.д.

Поздравляю! Вы очередной автор очередного г..нокода!

Что такое "чистый код"?
Как улучшить плохой код?
Почему чистый код часто "портится"?
Почему в написании кода так важны мелочи?

Читаем Роберт Мартин "Чистый Код"!

понедельник, 1 февраля 2010 г.

Заменяем механизм событий в CAB

Стандартный механизм событий в CompositeUI Application Block'е (CAB) не устраивает меня по нескольким причинам:
  1. Сложно и не удобно использовать. Нужно объявить событие, пометить его аттрибутом, потом сделать метод, который будет проверять на непустой список подписчиков и уже этот метод вызывать для генерации события.
  2. Отсутствие строгой типизации. Никто не мешает объявить подписку на событие с одной сигнатурой, а сгенерировать событие с другой.
  3. Неудобно отлаживать и разбираться в коде. Для того чтобы найти "подписчиков" и "публикаторов" события, приходится искать по строковому литералу.
  4. Неявная регистрация через рефлексию. А значит это не фатально, но медленнее чем могло бы быть.
Почитав Jeremy Miller's "Build Your Own CAB" и посмотрев на исходники StoryTeller, не мог удержаться от того, чтобы сделать что то подобное:

    public interface IListener<T> : IListener
    {
        void Handle(T message);
    }

    public interface IEventAggregator
    {
        void AddListener(IListener listener);
        void RemoveListener(IListener listener);
 
        void SendMessage<T>(T message);
        void SendMessage<T>() where T : new();
    }

После чего всё становится до неприличия простым:

    class MultiListener : IListener<Message1>, IListener<Message2>
    {
        public MultiListener(IEventAggregator eventAggregator)
        {
            // This is sample code!
            // Don't forget to unsubscribe in production code!
            eventAggregator.AddListener(this);
        }
 
        public void Handle(Message1 message)
        {
            Console.WriteLine("Message1");
        }
 
        public void Handle(Message2 message)
        {
            Console.WriteLine("Message2");
        }
    }

Исходный код можно взять здесь.

P.S. А мне начинает нравиться coding style, когда приватные методы начинаются с lower case.

P.P.S. До конца не уверен, что это окончательный вариант с "синхронизированными" "подписчиками". Что если потоков много и я хочу получать свои сообщения в том же потоке, в котором я подписывался? Надо обдумать...

среда, 20 января 2010 г.

Разделяй и тестируй!

Программирование это процесс написания кода. Причем большую часть этого времени мы изменяем существующий код. "Чужой" код. Чертыхаясь на того Васю Пупкина (оригинальное слово заменено, вдруг это прочтут дети...), который всё это написал, мы мучаемся, ставим "заплатки" и думаем как бы было здорово "всё переписать". Почему так? Почему так сложно разбираться в чужом коде, расширять его функциональность, не боясь ничего сломать? Уверены ли вы, что ваш код лучше, что вы не очередной "Вася Пупкин"? Говорите, что ваш код работает? Ну это еще не повод для самоуспокоения...

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

    public class OrderProcessingModule
    {
        public void SendInvoice(Guid orderId)
        {
            // get order from database
            Order order = GetOrder(orderId);
 
            // calculate total price
            double qtyPrice = 0;
            if (order.Qty < 10)
            {
                qtyPrice += order.Qty * order.UnitPrice;
            }
            else
            {
                // apply discount for large quantities
                qtyPrice += 0.9 * order.Qty * order.UnitPrice;
            }
 
            double shippingPrice = order.Shipping.IsInternational ? 10 : 5;
            if (order.Shipping.IsExpress)
                shippingPrice *= 2;
 
            var totalPrice = qtyPrice + shippingPrice;
 
            // prepare and send html-formatted message to customer
            var message = CreateMessageForOrder(order, totalPrice);
            var messagingGateway = new MessagingGateway();
            messagingGateway.Send(message);
        }
    }

Вроде бы неплохой код, структурирован, часть функциональности вынесена в отдельные методы:

        private Order GetOrder(Guid orderId)
        {
            string connectionString = ConfigurationManager.AppSettings["ConnectionString"];
            using (var connection = new SqlConnection(connectionString))
            {
                // 'SELECT * FROM Order' lives here
 
                var order = new Order();
                return order;
            }
        }
 
        private MailMessage CreateMessageForOrder(Order order, double totalPrice)
        {
            const string format = @"Define HTML template here";
            var sb = new StringBuilder();
            sb.AppendLine("" + order.UnitPrice + "");
            sb.AppendLine("" + order.Qty + "");
            sb.AppendLine("" + (order.Shipping.IsInternational ? "y/" : "n/") + (order.Shipping.IsExpress ? "y" : "n") + "");
            sb.AppendLine("" + totalPrice + "");
 
            var html = string.Format(format, order.OrderId, sb);
 
            var mailMessage = new MailMessage();
            mailMessage.Body = html;
 
            // ...
 
            return mailMessage;
        }

Но как это тестировать?! Как проверить что в зависимости от заказа правильно расчитывается его стоимость, что клиенту отправляется правильно отформатированное письмо и что оно отправляется правильному клиенту? Никак!!! Вариант с тем, чтобы написать тест, который будет ходить по SMTP или IMAP к почтовику, забирать письмо и проверять что оно существует и правильно сформированно я, по определенным причинам, не рассматриваю.

И что делать, если требования поменялись и теперь в логике расчета общей стоимости нужно учитывать персональную скидку клиента. Однако для этого придется внести изменения в код, который кроме расчетов стоимости еще и занят выборкой данных из БД, форматированием письма и его отправкой. В примере всё достаточно очевидно и просто, но жизнь сложнее - слишком много шансов что-нибудь случайно сломать.

Single Responsibility Principle - A class should have one, and only one, reason to change.

Звучит красиво, но что это на самом деле? На самом деле это означает, что код, который отвечает за расчет стоимости должен быть выделен в отдельный класс. И не только этот код...
Попробуем выделить в отдельные классы код, ответственный за чтение из базы, расчет стоимости и форматирование:

    class DataProvider
    {
        private readonly string _connectionString;
 
        public DataProvider(string connectionString)
        {
            _connectionString = connectionString;
        }
 
        public Order GetOrder(Guid orderId)
        {
            using (var connection = new SqlConnection(_connectionString))
            {
                // 'SELECT * FROM Order' lives here
 
                var order = new Order();
                return order;
            }
        }
    }

    class CostCalculator
    {
        public double CalculateOrderCost(Order order)
        {
            double qtyPrice = 0;
            if (order.Qty < 10)
            {
                qtyPrice += order.Qty * order.UnitPrice;
            }
            else
            {
                // apply discount for large quantities
                qtyPrice += 0.9 * order.Qty * order.UnitPrice;
            }
 
            double shippingPrice = order.Shipping.IsInternational ? 10 : 5;
            if (order.Shipping.IsExpress)
                shippingPrice *= 2;
 
            var totalPrice = qtyPrice + shippingPrice;
            return totalPrice;
        }
    }

    class MailFormatter
    {
        public MailMessage CreateMessageForOrder(Order order, double totalPrice)
        {
            const string format = @"Define HTML template here";
            var sb = new StringBuilder();
            sb.AppendLine("" + order.UnitPrice + "");
            sb.AppendLine("" + order.Qty + "");
            sb.AppendLine("" + (order.Shipping.IsInternational ? "y/" : "n/") + (order.Shipping.IsExpress ? "y" : "n") + "");
            sb.AppendLine("" + totalPrice + "");
 
            var html = string.Format(format, order.OrderId, sb);
 
            var mailMessage = new MailMessage();
            mailMessage.Body = html;
 
            // ...
 
            return mailMessage;
        }
    }

В результате наш OrderProcessingModule заметно упрощается:

    public class OrderProcessingModule
    {
        public void SendInvoice(Guid orderId)
        {
            // get order from database
            string connectionString = ConfigurationManager.AppSettings["ConnectionString"];
            Order order = new DataProvider(connectionString).GetOrder(orderId);
 
            // calculate total price
            var totalPrice = new CostCalculator().CalculateOrderCost(order);
 
            // prepare and send html-formatted message to customer
            var message = new MailFormatter().CreateMessageForOrder(order, totalPrice);
 
            var messagingGateway = new MessagingGateway();
            messagingGateway.Send(message);
        }
    }

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

Однако неплохо бы иметь возможность оттестировать и сам контроллер в лице OrderProcessingModule, но сейчас это невозможно, так как для этого нам потребуются конфигурационный файл с параметрами соединения, развернутая БД и сервис для отправки сообщений. Что ж, попробуем решить и эту проблему:

    public class OrderProcessingModule
    {
        private readonly IDataProvider _dataProvider;
        private readonly ICostCalculator _costCalculator;
        private readonly IMailFormatter _mailFormatter;
        private readonly IMessagingGateway _messagingGateway;
 
        public OrderProcessingModule(IDataProvider dataProvider, ICostCalculator costCalculator, IMailFormatter mailFormatter, IMessagingGateway messagingGateway)
        {
            if (dataProvider == null)
                throw new ArgumentNullException("dataProvider");
 
            // TODO: check all arguments
 
            _dataProvider = dataProvider;
            _costCalculator = costCalculator;
            _mailFormatter = mailFormatter;
            _messagingGateway = messagingGateway;
        }
 
        public void SendInvoice(Guid orderId)
        {
            // get order from database
            Order order = _dataProvider.GetOrder(orderId);
 
            // calculate total price
            var totalPrice = _costCalculator.CalculateOrderCost(order);
 
            // prepare and send html-formatted message to customer
            var message = _mailFormatter.CreateMessageForOrder(order, totalPrice);
            _messagingGateway.Send(message);
        }
    }

Теперь, вооружившись каким-нибудь Mock Framework'ом, можно приступать к написанию простых, понятных, быстрых юнит-тестов. Happy coding! :-)

понедельник, 11 января 2010 г.

Быстро или качественно?

Часто приходится слышать такое мнение, что для небольших проектов вся методология с testability, TDD, DI и прочее есть лишние затраты, отдаляющие время завершения проекта. Проблема в том, что заметить когда "небольшой" проект становится огромным запутанным "монстром" очень сложно и как правило в этот момент совсем нет времени на "внедрение" методологий, потому что кажется что "еще чуть-чуть, ну вот-вот..."

Jeremy Miller wrote about it:

I think you could argue with me that code quality doesn’t matter on small projects or projects that would be easier to rewrite later when and if they do need to change.  The only problem with that statement is that I’ve seen truly awful messes happen when those “throwaway” systems uncontrollably grew over time into big monsters.  My advice is to strive to reach a level of “unconscious competence” to where you naturally write high quality code and designs without going out of your way. 

вторник, 29 декабря 2009 г.

Опасайтесь долговой ямы!

Прочитал здесь интересное сравнение кода, написанного в стиле "как максимально быстро получить что то работающее", с банковским кредитом. Если время жизни вашей программы ограничено, допустим это курсовая работа "сдал-и-забыл" или это очередная программа из серии "Hello, world!" или какой то прототип, то такой подход позволяет воспользоваться главным приятным свойством всех кредитов - пользуйся сейчас, плати потом. Однако, если с течением времени продолжать писать, неотступая от выбранного стиля, объем кода растет, а с ним растут и "долговые обязательства". Обслуживание такого "кредита" становится тяжелой обузой, мешает нормальному развитию проекта, не дает совершать новые "покупки" в виде нового функционала.



В статье, кроме общего плана действий, дается хороший совет, которому каждый раз немного боязно следовать:

Have the courage and honesty to present the facts as they are. This seemingly risky approach boils down to the very human issues of accountability and trust.
Couch your argument like this: you’ve fielded successful software in the requested amount of time for a provisioned amount of money. In order to achieve this you’ve had to make compromises along the way in response to business pressures. Now, to go forward at a predictable and steady rate, you need to deal with the effects of these compromises. The entire organization has bought them, and now it’s time to pay back.

среда, 16 декабря 2009 г.

Будьте отзывчивыми!

Тестирование это инструмент для повышения качества программного обеспечения. Но "качественное" ПО - понятие многогранное и очень субъективное. Одним из таких критериев качества для конечного пользователя может быть "отзывчивый" пользовательский интерфейс.

Возьмем для примера простую форму:



Нажатие на каждую кнопку приводит к вызову соответсвующего метода у некоторого сервиса. Обычная такая форма, а вот и обычное для такого случая решение:


    public interface ISlowService
    {
        void Method1();
        void Method2();
        void Method3();
    }

    public class Presenter
    {
        public IView View { get; set; }

        public void ExecuteMethod1()
        {
            _service.Method1();
            View.AddMessage("Method1 complete.");
        }

        public void ExecuteMethod2()
        {
            _service.Method2();
            View.AddMessage("Method2 complete.");
        }

        public void ExecuteMethod3()
        {
            _service.Method3();
            View.AddMessage("Method3 complete.");
        }
    }

Запускаем, пробуем. Нажмем кнопку Method1, после чего понажимаем остальные кнопки, попробуем изменить размер окна, развернуть его или даже закрыть. Не работает?! Оказывается, что после нажатия на кнопку Method1, наша форма на одну минуту "замирает", потому что "выполняется" метод сервиса. "Так не бывает!" - скажете вы. Отчего ж... Одна минута - стандартный таймаут операции для WCF сервисов. Мало ли что там могло случиться на стороне сервиса. А пользователь ничего, подождет... Это и есть "обычное" решение такого рода задач.

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



У вас есть идеи чем могут заниматься 45 потоков в Outlook'е или 29 в Skype или в среднем по 30 "на брата" в инстансах IE? У меня нет таких разумных идей. Да, конечно, эти потоки в большинстве своем бездействуют, переключения между ними "стоят копейки" (в век гигагерцев то), однако если они бездействуют, то зачем их вообще создавать? Попробуем воспользоваться уже существующими в приложении потоками.

Немного изменим наш код:

        private delegate void ExecuteMethodDelegate();

        private void OnCallback(IAsyncResult ar)
        {
            var result = (AsyncResult) ar;
            var caller = (ExecuteMethodDelegate) result.AsyncDelegate;
            caller.EndInvoke(ar);

            var message = (string) ar.AsyncState;
            View.AddMessage(message);
        }

        public override void ExecuteMethod1()
        {
            ExecuteMethodDelegate d = Service.Method1;
            d.BeginInvoke(OnCallback, "Method1 complete.");
        }

        public override void ExecuteMethod2()
        {
            ExecuteMethodDelegate d = Service.Method2;
            d.BeginInvoke(OnCallback, "Method2 complete.");
        }

        public override void ExecuteMethod3()
        {
            Service.Method3();
            View.AddMessage("Method3 complete.");
        }

Мы воспользуемся асинхронными делегатами, которые в свою очередь используют потоки из ThreadPool, который уже существует и чей размер управляется managed runtime.

Запускаем, проверяем. После нажатия на кнопку Method1 мы можем нажимать на другие кнопки, нажатие на кнопку Method3 сразу же добавит строчку в список, делать с окном что угодно - разворачивать, минимизировать и даже закрыть окно и приложение. Однако, по истечении одной минуты ожидания, вместо строчки "Method1 complete." мы получим что то подобное:


Доступ к UI элементу из другого потока... Какие у нас есть варианты решения это проблемы?

  • Связка Control.InvokeRequired + Control.Invoke


        public void AddMessage(string message)
        {
            if (InvokeRequired)
            {
                Invoke(new MethodInvoker(() => AddMessage(message)));
                return;
            }

            listBox1.Items.Add(message);
        }


        Решение работает, однако если в нашем IView будет много методов/свойств, все они "обрастут" такими преамбулами, что не есть хорошо. Плюс, что если кто-то добавит новый метод, и забудет добавить преамбулу? Это может привести к неожиданным ошибкам с непонятной стратегией нахождения/тестирования. Нам нужно некоторое "централизованное" решение.


  • Попробовать "собрать" весь этот механизм внутри Presenter'а:
        private void OnCallback(IAsyncResult ar)
        {
            var result = (AsyncResult) ar;
            var caller = (ExecuteMethodDelegate) result.AsyncDelegate;
            caller.EndInvoke(ar);

            var message = (string) ar.AsyncState;

            var control = (Control) View;
            if (control.InvokeRequired)
                control.Invoke(new MethodInvoker(() => View.AddMessage(message)));
            else
                View.AddMessage(message);
        }


Один факт того, что Presenter'у нужно знать что его View это Control, останавливает меня от использования этого решения.
  • Использовать SynchronizationContext. Данный класс незаслужено игнорируется программистами, а зря...
        class UserState
        {
            public SynchronizationContext SynchronizationContext { get; set; }
            public string Message { get; set; }
        }

        private void OnCallback(IAsyncResult ar)
        {
            var result = (AsyncResult) ar;
            var caller = (ExecuteMethodDelegate) result.AsyncDelegate;
            caller.EndInvoke(ar);

            var userState = (UserState) ar.AsyncState;
            userState.SynchronizationContext.Post(obj => View.AddMessage(userState.Message), null);
        }

Имхо, элегантное решение, лишенное недостатков предыдущих двух. Мы получили "отзывчивый" интерфейс не затратив на это много сил. Попробуйте, не правда ли намного лучше, чем самое первое "обычное" решение? Так что мешает так делать всегда?...

Конечно и у данного решения есть недостатки, ограничивающие область его применения. Например невозможность отмены текущей выполняющейся операции. Это важно для ситуаций, когда операция ожидаемо будет продолжаться долго, например: копирование файлов, инсталяция приложения, расчет каких то сложных данных... Для таких случаев UI приложения должен содержать кнопку "Cancel" или какой то ее аналог. Но таких задач все же меньше, чем "кнопка, нажатие на которую приводит обращению к сервису far far away".

Сольюшн с примером можно взять здесь.

понедельник, 14 декабря 2009 г.

Тестирование UI с помощью White. Часть первая (окончание).

Потихоньку осваиваюсь... Уже умею вставлять в посты форматированный код!
В прошлый раз не получилось выложить сольюшн примера - исправляюсь.
Заодно и скриншот с результатами тестов:


Wider Two Column Modification courtesy of The Blogger Guide