вторник, 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 PrincipleA 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. 
Wider Two Column Modification courtesy of The Blogger Guide