1
\$\begingroup\$

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 static void RemoveAggregateStatistics(int variantId)
    {
       //remove logic here from Aggregate statistics table
    }
    public static void RemoveAdditionStatistics(int variantId)
    {
       //remove logic here from Addition statistics table
    }
}

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 ?
    {
        VariantRepo.RemoveAggregateStatistics(int variantId);
    }

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

public class AdditionCalculator : BaseVariant
{
    protected override void DeleteStep1Data()// Is it violating SRP ?
    {
        VariantRepo.RemoveAdditionStatistics(int variantId);
    }

    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 ?

\$\endgroup\$

closed as off-topic by t3chb0t, pacmaninbw, 200_success, yuri, IEatBagels May 10 at 18:00

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – t3chb0t, pacmaninbw, 200_success, yuri, IEatBagels
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 2
    \$\begingroup\$ // Is it violating SRP ? - for now it's violating Code Review's rules because this is hypothetical code and this is off-topic here. You either need to post the complete code or alternatively ask it on Software Engineering. \$\endgroup\$ – t3chb0t May 10 at 14:52
  • 1
    \$\begingroup\$ @t3chb0t Someone on SO suggested me to post this question on Code review as it is most suitable here and you are saying I should post on SE.Also I have posted those code which is required for the question because posting everything will make this question very very big.Thanks \$\endgroup\$ – ILoveStackoverflow May 13 at 5:43
  • \$\begingroup\$ I guess that someone doesn't know how Code Review works and gave you a wrong advice. You have to ask yourself what you really want? Is it about code? Then we obviously need code, you cannot remove the most of it and expect a review beceause we cannot review what we don't see. If you, however, don't wish a code review but other concepts that are not necessarily related to the implementation then SE is the place to ask. It's your choice. Here, we require complete and working code and you're always free to updte your question. \$\endgroup\$ – t3chb0t May 13 at 5:53
  • \$\begingroup\$ @t3chb0t I have posted most of the code required to help on my question and if required any further clarification or code then I am ready to include it in my question and also there is not syntactical error in the code.So could you please remove the ONHOLD tag from this question? \$\endgroup\$ – ILoveStackoverflow May 13 at 6:07
  • 1
    \$\begingroup\$ There are still a lot of placeholders like //save logic here or NotImplementedException. What happened to that code? \$\endgroup\$ – t3chb0t May 13 at 6:45