Way to hide logic behind class for better readability of method and refactor class to follow SRP

Question

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 ?

Answer 1

There are various problems with your code, eg the tight coupling to SqlConnection, and the fragile base class problem that you risk saddling yourself with by coupling Process to the child class methods DeleteStep1Data and Transform.

The problem with tightly coupling things to SqlConnection is that you make the code far harder to test: there has to be a connection to a database, in a know stable state, for you tests to pass and they likely cannot be run in parallel. In short, you can't unit test this code.

So I'd recommend eg passing in an ITransactionHandler instance that provides the transaction methods, BeginTransaction, Commit and Rollback that then act of an abstraction layer to the SQL Server classes at runtime and can be mocked in test. Likewise, IEntityVersion should be set by an injected value, not a new and the two items, VariantRepo and categoryRepo look suspiciously like they are static classes and so again should be abstracted and injected.

Regarding the fragile base class problem, this is the problem with using inheritance; so don't use inheritance. Have something like the following instead:

public interface IVariantProcessor
{
    void Process(Variant model);
}

public sealed class VariantProcessor : IVariantProcessor
{
    private readonly Action _transform;
    private readonly Action _deleteStep1Data;

    public Variant(Action transform, Action deleteStep1Data)
    {
        _transform = transform;
        _deleteStep1Data = deleteStep1Data;
    }

    public void Process(Variant model)
    {
        try
        {
            _transform();
            using (var myConnection ...)
            using (var transaction ...)
            {
                try { ... }
                catch (Exception)
                {
                    _deleteStep1Data();
                }
            }
        }
        catch (Exception) {}
    }
}

public class AggregateCalculator : IVariantProcessor
{
    public void Process(Variant model) 
        => new VariantProcessor(Transform, DeleteStep1Data).Process();

    private void DeleteStep1Data()
    {
        ...
    }

    private void Transform()
    {
        ...
    }
}

That way, you aren't coupling that VariantProcessor to the XXXCalculator classes; you are using composition rather than inheritance.

Whether you have a problem with the SRP is hard to tell when your methods as shown just throw NotImplementedException and thus violate the Liskov substitution principle instead (I appreciate that this is just example code though). What happens in the real code may or may not violate the SRP. But just having two (presumably related) methods in a class isn't a violation in itself.