3

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 ?

3

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.

  • Currently I am using SQL SERVER database to store data hence SqlConnection class and also there is some logic inside transform method but i thought may be its not relevant hence i didnt posted it.I didnt understand your point regarding base class.Could you please share some ideas around designing base class and around derived class for 2 steps please – ILoveStackoverflow May 13 at 8:17
  • @ILoveStackoverflow, I have updated my answer to address your comment. – David Arno May 13 at 9:37
  • Sorry actually I have understood.Is it possible to decouple Versioning part because i feel like that code is tightly coupled with my base class and tomorrow it might be possible that i completely remove this versioning part.So is it possible to design it in a way that it doesnt affect my code ? – ILoveStackoverflow May 13 at 10:36
  • 1
    Yes, you replace the line private readonly IEntityVersion _entityVersion = new EntityVersion(); with private readonly IEntityVersion _entityVersion; and you set it via the constructor (ie you inject it). – David Arno May 14 at 11:31
  • Thank you so much.With the help of your code design I tried to refactor my code but got stuck with the problem describe here in case if you have little bit of time to look at it please : stackoverflow.com/questions/56128836/… – ILoveStackoverflow May 14 at 11:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.