Refaktoringi codzienne

Czy na pewno nie mamy czasu na refaktoryzację?

Piotr Wandycz

Refaktoryzacja kojarzy się z wielkim, długotrwałym procesem przepisywania pewnej części, albo nawet całości aplikacji. Natomiast w mojej głowie to proces normalny, codzienny. Masz lepszą nazwę dla danego pola, bądź metody? Zmień ją, nie zastanawiaj się. Widzisz niepotrzebny komentarz? To go usuń. A co, jeśli możesz pozbyć się komentarza poprzez zastosowanie bardziej opisowej nazwy metody? Czy wykonasz tę refaktoryzację? Na swojej drodze spotykałem osoby, które miały pomysły na lepsze nazwy, a mimo to zostawiały stare. Proszę – nie stań się jedną z takich osób. Stosuj tak zwaną regułę skauta – zostaw obozowisko kod trochę lepszym, niż go zastałeś/-łaś.

Na początku swojej programistycznej kariery pisanie małych metod może być trudne. Po dziś dzień prześladuje mnie program napisany w trakcie trwania studiów dla klienta, gdzie plik MainWindow.cs ma 4000 linii kodu. Niestety duże funkcje powodują kilka poważnych problemów. Czasami jest to kod niedający się zrozumieć na pierwszy rzut oka i wymagający głębszej analizy. Zdarza się od tego gorszy przypadek – ukryta/zaciemniona logika. Tylko pozornie wiemy, co robi metoda, ale gdy wdrążymy się w nią głębiej – zaczynamy widzieć, że zachowuje się zupełnie inaczej, niż powinna. Najpierw puchną nam metody, później klasy, a z tych po czasie powstają legendarne „ośmiotysięczniki”. Tych problemów dałoby się uniknąć, stosując jedną lub dwie banalnie proste techniki refaktoryzacji. Mimo iż mogą wydawać się naturalne, to mają swoje odpowiednie nazwy.

Spójrz na przykładowy kod. Czy w ciągu minuty będziesz w stanie dokładnie wytłumaczyć, co się dzieje w metodzie Handle, mając pewność, że nic nie przeoczyłeś/-łaś?

using Dapper;
using MediatR;
using System;
using System.Collections.Generic;
using System.Data.SqlClient;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Teacher.Domain.DomainObjects;
using Teacher.Domain.Infrastructure;
using Teacher.Website.Infrastructure;

[assembly: InternalsVisibleTo("Teacher.Website.Feature.Tests")]
namespace Teacher.Website.Feature.Interview.Answer
{
    internal class QueryHandler : IRequestHandler<Query, ViewModel>
    {
        private readonly IConnectionStringFactory _connectionStringFactory;
        private readonly List<AnswerOccurence> _occurences;

        public QueryHandler(IConnectionStringFactory connectionStringFactory)
        {
            _connectionStringFactory = connectionStringFactory;
            _occurences = new List<AnswerOccurence>();
        }

        public async Task<ViewModel> Handle(Query query, CancellationToken cancellationToken)
        {
            var model = new ViewModel();
            IEnumerable<int> questionIds;
            using (var db = new SqlConnection(_connectionStringFactory.ToDatabase()))
            {
                var sql = $"SELECT Id FROM [Interview].[Question] WHERE [Answer] IS NOT NULL";
                questionIds = await db.QueryAsync<int>(sql);
            }
            IEnumerable<AnswerReadModel> answersDb;
            using (var db = new SqlConnection(_connectionStringFactory.ToDatabase()))
            {
                var sql = $"SELECT Id, QuestionId, UserId, AnsweredAt FROM [Interview].[Answer]";
                answersDb = await db.QueryAsync<AnswerReadModel>(sql);
            }
            var answers = answersDb.Select(x => new UserAnswer(x.QuestionId, x.AnsweredAt));

            foreach (var answer in answers)
            {
                if (!_occurences.Any(x => x.QuestionId == answer.QuestionId))
                {
                    _occurences.Add(new AnswerOccurence(answer.QuestionId, answer.AnsweredAt));
                    continue;
                }
                _occurences.Single(x => x.QuestionId == answer.QuestionId).AddOccurence(answer.AnsweredAt);
            }

            int nextQuestionId;
            if (!questionIds.Any())
                throw new DomainException("List of question ids is empty in QuestionSelector");

            if (!_occurences.Any())
                nextQuestionId = questionIds.OrderBy(x => new Random().NextDouble()).First();
            else
            {
                var notAnsweredQuestions = questionIds.Except(_occurences.Select(x => x.QuestionId));
                var rarelyAnsweredQuestions = _occurences.Where(x => x.AnsweredTimes < 3).Select(x => x.QuestionId);

                nextQuestionId = notAnsweredQuestions.Union(rarelyAnsweredQuestions).OrderBy(x => new Random().NextDouble()).First();
            }

            vw_QuestionDetails questionDb;
            using (var db = new SqlConnection(_connectionStringFactory.ToDatabase()))
            {
                var sql = $"SELECT * FROM [Interview].[vw_QuestionDetails] WHERE [QuestionId] = { nextQuestionId }";
                questionDb = await db.QueryFirstAsync<vw_QuestionDetails>(sql);
            }
            model.Question = new QuestionReadModel
            {
                Id = questionDb.QuestionId,
                CategoryName = questionDb.CategoryName,
                Content = questionDb.Content,
                Answer = questionDb.Answer
            };
            model.Answer.QuestionId = model.Question.Id;
            return model;
        }
    }
}

Moglibyśmy zastosować tu dwa przekształcenia refaktoryzacyjne – extract method i compose method. Po pierwsze – jeśli widzisz parę linijek kodu, które są ze sobą logicznie związane, to wyodrębnij metodę. Ot i cała definicja extract method. W compose method natomiast chodzi o to, że metoda jest skomponowana z wywołań innych metod. Bardzo lubię, kiedy mogę popatrzeć na funkcję Handle i w miarę szybko zorientować się co tam się dzieje. Spójrz jeszcze raz na tę samą klasę po refaktoryzacji – nie zmieniła się ilość zależności, po prostu doszło trochę prywatnych metod. Czy nie jest to dla Ciebie czytelniejsze?

using Dapper;
using MediatR;
using System;
using System.Collections.Generic;
using System.Data.SqlClient;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Teacher.Domain.DomainObjects;
using Teacher.Domain.Infrastructure;
using Teacher.Website.Infrastructure;

[assembly: InternalsVisibleTo("Teacher.Website.Feature.Tests")]
namespace Teacher.Website.Feature.Interview.Answer
{
    internal class QueryHandler : IRequestHandler<Query, ViewModel>
    {
        private readonly IConnectionStringFactory _connectionStringFactory;
        private readonly List<AnswerOccurence> _occurences;

        public QueryHandler(IConnectionStringFactory connectionStringFactory)
        {
            _connectionStringFactory = connectionStringFactory;
            _occurences = new List<AnswerOccurence>();
        }

        public async Task<ViewModel> Handle(Query query, CancellationToken cancellationToken)
        {
            var model = new ViewModel();
            var questionIds = await GetQuestionIdsThatHasAnswerAsync();
            var answersDb = await GetAnswersAsync(query.UserId);
            var answers = MapAnswers(answersDb);
            var occurences = GetData(answers);
            var nextQuestionId = GetNextQuestionId(questionIds, occurences);
            var questionDb = await GetQuestionAsync(nextQuestionId);
            model.Question = MapQuestion(questionDb);
            model.Answer.QuestionId = model.Question.Id;
            return model;
        }

        private async Task<IEnumerable<int>> GetQuestionIdsThatHasAnswerAsync()
        {
            using (var db = new SqlConnection(_connectionStringFactory.ToDatabase()))
            {
                var sql = $"SELECT Id FROM [Interview].[Question] WHERE [Answer] IS NOT NULL";
                return await db.QueryAsync<int>(sql);
            }
        }

        private async Task<IEnumerable<AnswerReadModel>> GetAnswersAsync(int userId)
        {
            using (var db = new SqlConnection(_connectionStringFactory.ToDatabase()))
            {
                var sql = $"SELECT Id, QuestionId, UserId, AnsweredAt FROM [Interview].[Answer]";
                return await db.QueryAsync<AnswerReadModel>(sql);
            }
        }

        private IEnumerable<UserAnswer> MapAnswers(IEnumerable<AnswerReadModel> answersReadModel)
        {
            return answersReadModel.Select(x => new UserAnswer(x.QuestionId, x.AnsweredAt));
        }

        private IEnumerable<AnswerOccurence> GetData(IEnumerable<UserAnswer> answers)
        {
            foreach (var answer in answers)
            {
                if (!_occurences.Any(x => x.QuestionId == answer.QuestionId))
                {
                    _occurences.Add(new AnswerOccurence(answer.QuestionId, answer.AnsweredAt));
                    continue;
                }
                _occurences.Single(x => x.QuestionId == answer.QuestionId).AddOccurence(answer.AnsweredAt);
            }
            return _occurences;
        }

        private int GetNextQuestionId(IEnumerable<int> questionIds, IEnumerable<AnswerOccurence> occurences)
        {
            if (!questionIds.Any())
                throw new DomainException("List of question ids is empty in QuestionSelector");

            if (!occurences.Any())
                return GetRandomInt(questionIds);

            var questionPool = BuildQuestionPool(questionIds, occurences);
            return GetRandomInt(questionPool);
        }

        private IEnumerable<int> BuildQuestionPool(IEnumerable<int> questionIds, IEnumerable<AnswerOccurence> occurences)
        {
            var notAnsweredQuestions = questionIds.Except(occurences.Select(x => x.QuestionId));
            var rarelyAnsweredQuestions = occurences.Where(x => x.AnsweredTimes < 3).Select(x => x.QuestionId);
            return notAnsweredQuestions.Union(rarelyAnsweredQuestions);
        }

        private int GetRandomInt(IEnumerable<int> collection)
        {
            return collection.OrderBy(x => new Random().NextDouble()).First();
        }

        private async Task<vw_QuestionDetails> GetQuestionAsync(int questionId)
        {
            using (var db = new SqlConnection(_connectionStringFactory.ToDatabase()))
            {
                var sql = $"SELECT * FROM [Interview].[vw_QuestionDetails] WHERE [QuestionId] = { questionId }";
                return await db.QueryFirstAsync<vw_QuestionDetails>(sql);
            }
        }

        private QuestionReadModel MapQuestion(vw_QuestionDetails question)
        {
            return new QuestionReadModel
            {
                Id = question.QuestionId,
                CategoryName = question.CategoryName,
                Content = question.Content,
                Answer = question.Answer
            };
        }
    }
}

Jeśli pierwszy raz widzisz taki kod, to analiza co tam się dzieje pewnie i tak chwilę potrwa. Ale jeśli to Ty go napisałeś/-łaś, to nie będziesz mieć problemów, gdy do niego wrócisz nawet po roku. Dodatkowo posiadanie takich małych metod pozwala nam na pogrupowanie ich w odpowiednie klasy i wypchnięcie tych odpowiedzialności na zewnątrz. Ja tutaj odwracałem proces refaktoryzacji – zamieniałem kod z dołu strony na ten powyżej. Niestety w realnym świecie to nie jest takie proste, dlatego musimy pilnować wydzielania metod w momencie ich pisania i nieodkładania tego na później, szukania wymówek typu “bo inne rzeczy nas gonią”. Po przeniesieniu funkcji poza QueryHandler całość prezentuje się następująco:

using MediatR;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Teacher.Domain.CalculateAnswerOccurences;
using Teacher.Domain.DomainObjects;
using Teacher.Domain.SelectQuestions;

[assembly: InternalsVisibleTo("Teacher.Website.Feature.Tests")]
namespace Teacher.Website.Feature.Interview.Answer
{
    internal class QueryHandler : IRequestHandler<Query, ViewModel>
    {
        private readonly IRepository _repository;
        private readonly IQuestionSelector _questionSelector;
        private readonly IAnswerOccurenceCalculator _answerOccurenceCalculator;

        public QueryHandler(IRepository repository, 
            IQuestionSelector questionSelector, 
            IAnswerOccurenceCalculator answerOccurenceCalculator)
        {
            _repository = repository;
            _questionSelector = questionSelector;
            _answerOccurenceCalculator = answerOccurenceCalculator;
        }

        public async Task<ViewModel> Handle(Query query, CancellationToken cancellationToken)
        {
            var model = new ViewModel();
            var questionIds = await _repository.GetQuestionIdsThatHasAnswerAsync();
            var answersDb = await _repository.GetAnswersAsync(query.UserId);
            var answers = MapAnswers(answersDb);
            var occurences = _answerOccurenceCalculator.GetData(answers);
            var nextQuestionId = _questionSelector.GetNextQuestionId(questionIds, occurences);
            var questionDb = await _repository.GetQuestionAsync(nextQuestionId);
            model.Question = MapQuestion(questionDb);
            model.Answer.QuestionId = model.Question.Id;
            return model;
        }

        private IEnumerable<UserAnswer> MapAnswers(IEnumerable<AnswerReadModel> answersReadModel)
        {
            return answersReadModel.Select(x => new UserAnswer(x.QuestionId, x.AnsweredAt));
        }

        private QuestionReadModel MapQuestion(vw_QuestionDetails question)
        {
            return new QuestionReadModel
            {
                Id = question.QuestionId,
                CategoryName = question.CategoryName,
                Content = question.Content,
                Answer = question.Answer
            };
        }
    }
}

Czy ten kod jest idealny? Nie, nie jest. Ale mogę w miarę szybko zrozumieć co się w nim dzieje. Można by tu jeszcze użyć AutoMappera i pozbyć się pozostałych dwóch metod prywatnych. Mnie nie przeszkadza ręczne mapowanie, dlatego zostawiam dzielenie na klasy w tym momencie, bo ten kod łapie się u mnie pod definicję “dostatecznie dobrego” (“good enough”). Refaktoryzacja nie zawsze musi kojarzyć się od razu z zaoraniem konkretnego modułu w aplikacji. Możemy przeprowadzać ją klasa po klasie, poświęcając na nią kilka/kilkanaście minut dziennie. Nawet jeśli wydaje się być stratą czasu, bo “co da poprawienie jednej klasy na tydzień?”, to konsekwentnie doprowadzi do tego, że nauczysz się od razu pisać kod, który jest wystarczająco dobry.

Dodaj komentarz

Twój adres email nie zostanie opublikowany. Pola, których wypełnienie jest wymagane, są oznaczone symbolem *