Как скрыть логику за классом, чтобы улучшить читаемость метода и класса рефакторинга, чтобы следовать SRP? - PullRequest
Новая
       35

Как скрыть логику за классом, чтобы улучшить читаемость метода и класса рефакторинга, чтобы следовать SRP?

10 голосов
/ 07 мая 2019

У меня есть алгоритм для создания версии для сущности, а затем я сохраняю эту версию для сущности ниже 2:

1) Вариант

2) Категория 

interface IEntityVersion
{
    string GetVersion();
}

public class EntityVersion : IEntityVersion
{
    public string GetVersion()
    {
        return null;
    }
}

public interface IVariant
{
    void Process(Variant model, string connectionString);
}

public abstract class BaseVariant : IVariant
{
    private readonly IEntityVersion _entityVersion = new EntityVersion();

    public void Process(Variant model, string connectionString)
    {
        try
        {
            Transform();
            string variantVersion = _entityVersion.GetVersion();
            using (var myConnection = new SqlConnection(connectionString))
            {
                myConnection.Open();
                using (var transaction = myConnection.BeginTransaction())
                {
                    try
                    {
                        VariantRepo.UpdateVariantVersion(
                          myConnection,
                          transaction, model.VariantId, variantVersion);

                        CategoryRepo.UpdateCategoryVariantMapping(
                                     myConnection,
                                     transaction, model.CategoryId, variantVersion);

                        transaction.Commit();
                    }
                    catch (Exception)
                    {
                        transaction.Rollback();
                        DeleteStep1Data();
                    }
                }
            }
        }
        catch (Exception)
        {
            //log error
        }
    }

    protected abstract void DeleteStep1Data();
    protected abstract void Transform();
}

public class Variant
{
    public int VariantId { get; set; }
    public int CategoryId { get; set; }
}

public class VariantRepo
{
    public static void UpdateVariantVersion(SqlConnection sqlConnection,
        SqlTransaction transaction, int variantId, string version)
    {
        //save logic here
    }
}

public class CategoryRepo
{
    public static void UpdateCategoryVariantMapping(SqlConnection sqlConnection,
        SqlTransaction transaction, int categoryId, string version)
    {
        //save logic here
    }
}

У меня есть 2 производных типа (AggregateCalculator и AdditionCalculator), каждый из которых имеет собственную реализацию методов Transform и DeleteStep1Data. 

public class AggregateCalculator : BaseVariant
{
    protected override void DeleteStep1Data() // Is it violating SRP ?
    {
        throw new NotImplementedException();
    }

    protected override void Transform()
    {
        throw new NotImplementedException();
    }
}

public class AdditionCalculator : BaseVariant
{
    protected override void DeleteStep1Data()// Is it violating SRP ?
    {
        throw new NotImplementedException();
    }

    protected override void Transform()
    {
        throw new NotImplementedException();
    }
}

Мне кажется, что метод Process делаетслишком много работы, и если можно было бы скрыть связанную с сохранением версии логику за классом EntityVersion, чтобы метод Process выглядел просто.

Step1 и Step2 синхронизированы, так что еслив Step2 есть ошибка, я вызываю метод DeleteStep1Data для удаления всех данных, сохраненных в Step1.

Также я чувствую, что мои 2 производных класса AggregateCalculator и AdditionCalculator обрабатываютболее 1 ответственности, т.е. выполнение преобразования, а также удаление данных, сохраненных в процессе преобразования, хотя я не уверен, верно ли это или нет.

Существует ли возможность рефакторинга кода выше для улучшения читабельности иобрабатывать SRP?

1 Ответ

5 голосов
/ 09 мая 2019

Пытаясь понять ваш вопрос

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

Давайте предположим, что ваша сущность car, а категории для этой сущности: Toyota, BMWи Nissan.Теперь ваша сущность, скажем, «Toyota Corona с id = 123», изменилась.Зачем вам нужно отслеживать изменения в категории?Разве вы не можете просто сказать, что сущность с id = 123 изменилась?

Нарушение SRP

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

У вас есть класс с именем AggregateCalculator, я предполагаю, что основная ответственность этого класса заключается в вычислении агрегации, которая происходит в методе Transform().Теперь вам нужно выполнить 2 шага внутри Transform().Это не обязательно является нарушением SRP ... потому что на более высоком уровне ваш калькулятор совокупности делает одну вещь: вычисляет агрегацию.

Вы можете искать общие признаки нарушения SRP:

  1. Согласно 2-му закону Маловича Николы Маловича :

    Любой класс, имеющий более 3 зависимостей, должен быть допрошен за нарушение SRP

  2. Если размер вашего класса слишком велик, вам нужно расспросить его о нарушении SRP.

Нарушение СУХОГО

Оба ваших класса: AggregateCalculator и AdditionCalculator выполняют свои вычисления в 2 этапа, step-1 и step-2 ... и у вас есть общий метод: DeleteStep1Data() в обоих классах для удаления step-1 , если step-2 не удается ...Я предполагаю, что реализация DeleteStep1Data() отличается для каждого из этих классов, но я чувствую, что он все еще содержит дублированный код (не DRY).Можно утверждать, что это также нарушает SRP, потому что AggregateCalculator отвечает как за: вычисление агрегации, так и за «зеркалирование транзакции с БД» (это трудно сказать, не видя ваш полный код).

Похоже, что оба step-1 и step-2 являются транзакциями БД, поэтому другой подход заключался бы в том, чтобы поместить оба шага в одну транзакцию БД ... например, вы можете написать хранимую процедуру, напримерэто: 

CREATE PROCEDURE AggregateCalculationSP
AS
BEGIN
    BEGIN TRANSACTION t1
        BEGIN TRY
            -- do step 1
            -- do step 2
        END TRY
        BEGIN CATCH
            ROLLBACK TRANSACTION t1
        END CATCH
    COMMIT TRANSATION t1
END

Теперь вы можете взять DeleteStep1Data() из вашего класса.

...