How to hide logic behind a class to improve readability of method and refactor class to follow SRP?

.net, c#, refactoring, single-responsibility-principle

Solution

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:

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

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

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.

Problem

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 ?

Original source