How to hide logic behind a class to improve readab

问题:

I have an algorithm to create a version for an entity and then I save that version against below 2 entity:

1) Variant

2) Category

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
    }
}

I have 2 derived types(AggregateCalculator and AdditionCalculator) each having their own implementation of Transform and DeleteStep1Data methods.

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();
    }
}

I feel like the Process method is doing too much work and if it would be possible to hide version saving related logic behind the EntityVersion class so that the Process method looks simple.

Step1 and Step2 are in sync so that if there is an error in Step2, I call the DeleteStep1Data method to delete all the data saved in Step1.

Also I feel like my 2 derived classes AggregateCalculator and AdditionCalculator are handling more than 1 responsibility, i.e. running a transformation and also deleting the data stored during the transformation process, although I am not sure if this is true or not.

Is there a possibility to refactor above code to improve readability and handle SRP ?

回答1:

Trying to understand your question

You have an entity... when the entity changes, you want to create a change-version for your entity. What is not clear to me is that why this change needs to be tracked against both variant and category?

Let's assume your entity is car and the categories for that entity are: Toyota, BMW and Nissan. Now your entity, let's say "Toyota Corona with id = 123" is changed. Why do you need to track the change against the category? Can't you just say that entity with id = 123 has changed?

Violation of SRP

As I mentioned in the comment, since you have left out part of your logic, it's hard to understand if your code if violating SRP or not, but I can give you some general suggestions:

You have a class called AggregateCalculator, I assume the main responsibility of this class is to calculate aggregation which happens in Transform() method. Now you require to execute 2 steps inside Transform(). This is not necessarily a violation of SRP... because from a higher level, your aggregate calculator does one thing: calculate aggregation.

You can look for general signs of SRP violation:

1. According to Nikola Malovic's 2nd law of IoC:

   Any class having more than 3 dependencies should be questioned for SRP violation

2. If the size of your class is too big, then you need to question it for SRP violation.

Violation of DRY

Both of your classes: AggregateCalculator and AdditionCalculator do their calculation in 2 steps, step-1 and step-2... and you have a common method: DeleteStep1Data() in both classes to delete step-1 if step-2 fails... I assume the implementation of DeleteStep1Data() is different for each of these classes, but I feel it still contains duplicated code (not DRY). One may argue that this violates SRP too, because AggregateCalculator is responsible for both: Calculating Aggregation and "Mirroring a DB Transaction" (this is hard to tell without seeing your complete code).

It appears that both step-1 and step-2 are DB transactions, so a different approach would be to put both steps inside a single DB transaction... for example you can write a stored procedure like this:

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

Now you can take out DeleteStep1Data() from your class.