1

Context:

Creating a "middleware" between 2 services I have to Get data from Service Source and get it into Service ERP. There is a multiple type of Data: A, B, C. An integration process follows the following script:

  • Get pending ID for A
  • Get A for each of those Id
  • For each of those A convert them into ERP A.
  • Submit the A to the ERP.
  • If any pb occurred cancel this A.
  • Else Valid this A.

With a little Log and Error handling around each of those steps. The process stays the same for A, B and C.
I would like to refactor to avoid repetition and having to modify multiple method for a simple change in the overall behavior.

class Program
{
    static IServiceMock_Source ServiceSource;
    static IServiceMock_ERP ServiceERP;
    
    static void Main(string[] args)
    {
        Integrator_A();

        //Integrator_B();
        //Integrator_C();
    }

    public static bool Integrator_A()
    {
        var As = GetPending_A();

        if (!As.Any())
        {
            Logger.Log(typeof(Program),
                Level.Debug, "There is no As to integrate", null);
            return false;
        }
        Creator_A(As.ToArray());
        return true;
    }
    private static List<A_Entity> GetPending_A()
    {
        var As = new List<A_Entity>();

        int[] AsIds = null;
        try
        {
            AsIds = ServiceSource.GetA_Pending();
        }
        catch (Exception e)
        {
            Logger.Log(typeof(Program),
               Level.Error,
                $"ERR ServiceSource.GetA_Pending : " + e
               , e);
        }

        if (AsIds.Any())
        {
            Logger.Log(typeof(Program),
                Level.Info,
                $"AsIds : [{string.Join(", ", AsIds)}]"
                , null);
        }

        foreach (var id in AsIds)
        {
            A_Entity tempA;
            try
            {
                tempA = ServiceSource.GetA(id);
                As.Add(tempA);
            }
            catch (Exception e)
            {
                Logger.Log(typeof(Program),
                 Level.Error,
                  $"ERR ServiceSource.GetA, Impossible to get l'ID[{id}] : " + e
                 , e);
            }
        }
        return As;
    }
    private static void Creator_A(params A_Entity[] a_Entities)
    {
        foreach (var entity in a_Entities)
        {
            var isACreated = CreateAInERP(entity, out string error);

            if (!isACreated)
            {
                var err = $"Failed A creation" +
                    $"[{entity.A_EntityEntityDbId}, entity.otherId, {entity.ProcessableEntityDbId}] [..]" +
                    $"\nError : \n{error}";
                Logger.Log(typeof(Program), Level.Error, err, null);

                ServiceSource.CancelA(entity.A_EntityEntityDbId, true);

                var source = $"MachineName:{System.Environment.MachineName}" +
                    $", App:{System.AppDomain.CurrentDomain.FriendlyName}" +
                    $", Path:{Environment.GetCommandLineArgs()[0]}"
                    ;

                //ServiceSource.CreateErrorMessage(new ErrorMessageDTO
                //{
                //    ProcessID = entity.ProcessInformation.ProcessInformationId,
                //    Source = source.Truncate(500),
                //    Category = this.GetType().FullName,
                //    Query = "CreateAInERP()",
                //    Message = err,
                //});
            }
            ServiceSource.ValideA(entity.A_EntityEntityDbId, isACreated);
        }
    } 
    private static bool CreateAInERP(A_Entity entity, out string error)
    {
        error = "";
        A_In erpItem = null;
        try
        {
            erpItem = Converter.ToERP(entity);
        }
        catch (Exception e)
        {
            error = "Erreur projection: CreateAInERP." +
                 $" DB_id = {entity.A_EntityEntityDbId}." + e;
            return false;
        }

        A_Response result;
        try
        {
            result = ServiceERP.Submit_A(
               new A_Request
               {
                   Context = new Context { },
                   A_In = erpItem,
               });
        }
        catch (Exception e)
        {// Timeout and service exception
            error = "Error Integration ERP: Submit_A." +
               $" DB_id = {entity.A_EntityEntityDbId}." + e;
            return false;
        }

        if (result.ErrorCode != "OK")
        {// bizness Error
            error = "Error Integration: " +
                $"DB_id = {entity.A_EntityEntityDbId}. " +
                $"[{result.ErrorCode}] : result.errorMsg";
            return false;
        }
        return true;
    }
}

public class Converter
{
    public static A_In ToERP(A_Entity entity)
    { // Complexe mapping of ERP entities
        return new A_In();
    }
    public static B_In ToERP(B_Entity entity)=> new B_In();
    // internal static C_In ToERP(C_Entity entity)=> new C_In();
}

This code as a huge repetition between process A and B.
You will notice that the following code is 100% the same as A. With only a type difference.
In fact the real code for B,C,D are coded by copy pasting A block. And using Ctrl+R+R for rename just a few times.

public static bool Integrator_B()
{
    var Bs = GetPending_B();

    if (!Bs.Any())
    {
        Logger.Log(typeof(Program),
            Level.Debug, "There is no Bs to integrate", null);
        return false;
    }
    Creator_B(Bs.ToArray());
    return true;
}
private static List<B_Entity> GetPending_B()
{
    var Bs = new List<B_Entity>();

    int[] BsIds = null;
    try
    {
        BsIds = ServiceSource.GetB_Pending();
    }
    catch (Exception e)
    {
        Logger.Log(typeof(Program),
            Level.Error,
            $"ERR ServiceSource.GetB_Pending : " + e
            , e);
    }

    if (BsIds.Any())
    {
        Logger.Log(typeof(Program),
            Level.Info,
            $"BsIds : [{string.Join(", ", BsIds)}]"
            , null);
    }

    foreach (var id in BsIds)
    {
        B_Entity tempB;
        try
        {
            tempB = ServiceSource.GetB(id);
            Bs.Add(tempB);
        }
        catch (Exception e)
        {
            Logger.Log(typeof(Program),
                Level.Error,
                $"ERR ServiceSource.GetB, Impossible to get ID[{id}] : " + e
                , e);
        }
    }
    return Bs;
}
private static void Creator_B(params B_Entity[] b_Entities)
{
    foreach (var entity in b_Entities)
    {
        var isBCreated = CreateBInERP(entity, out string error);

        if (!isBCreated)
        {
            var err = $"Failed B creation" +
                $"[{entity.B_EntityEntityDbId}, entity.otherId, {entity.ProcessableEntityDbId}] [..]" +
                $"\nError : \n{error}";
            Logger.Log(typeof(Program), Level.Error, err, null);

            ServiceSource.CancelB(entity.B_EntityEntityDbId, true);

            var source = $"MachineName:{System.Environment.MachineName}" +
                $", App:{System.AppDomain.CurrentDomain.FriendlyName}" +
                $", Path:{Environment.GetCommandLineArgs()[0]}"
                ;

            //ServiceSource.CreateErrorMessage(new ErrorMessageDTO
            //{
            //    ProcessID = entity.ProcessInformation.ProcessInformationId,
            //    Source = source.Truncate(500),
            //    Category = this.GetType().FullName,
            //    Query = "CreateBInERP()",
            //    Message = err,
            //});
        }
        ServiceSource.ValideB(entity.B_EntityEntityDbId, isBCreated);
    }
}
private static bool CreateBInERP(B_Entity entity, out string error)
{
    error = "";
    B_In erpItem = null;
    try
    {
        erpItem = Converter.ToERP(entity);
    }
    catch (Exception e)
    {
        error = "Erreur projection: CreateBInERP." +
                $" DB_id = {entity.B_EntityEntityDbId}." + e;
        return false;
    }

    B_Response result;
    try
    {
        result = ServiceERP.Submit_B(
            new B_Request
            {
                Context = new Context { },
                B_In = erpItem,
            });
    }
    catch (Exception e)
    {// Timeout and service exception
        error = "Error Integration ERP: Submit_B." +
            $" DB_id = {entity.B_EntityEntityDbId}." + e;
        return false;
    }

    if (result.ErrorCode != "OK")
    {// bizness Error
        error = "Error Integration: " +
            $"DB_id = {entity.B_EntityEntityDbId}. " +
            $"[{result.ErrorCode}] : result.errorMsg";
        return false;
    }

    return true;    
}

Here is the code use for the mockup. It's out of modification Scope.
But it's needed to have no compilation error in this MRE.

public interface IServiceMock_ERP
{
    public A_Response Submit_A(A_Request request);
    public B_Response Submit_B(B_Request request);
}

public class A_Request
{
    public Context Context { get; set; }
    public A_In A_In { get; set; }
}
public class B_Request
{
    public Context Context { get; set; }
    public B_In B_In { get; set; }
}
public class Context { }
public class A_In
{
    public string RealDataHere { get; set; }
}
public class B_In
{
    public string RealDataHere { get; set; }
}
public class A_Response
{
    public string ErrorCode { get; set; }
    public A_Out A_Out { get; set; }
}
public class B_Response
{
    public string ErrorCode { get; set; }
    public B_Out B_Out { get; set; }
}
public class A_Out
{
    public string Error { get; set; }
}
public class B_Out
{
    public string Error { get; set; }
}

public interface IServiceMock_Source
{
    public int[] GetA_Pending();
    public A_Entity GetA(int id_A);
    public bool CancelA(int id_A, bool value);
    public bool ValideA(int id_A, bool value);

    public int[] GetB_Pending();
    public B_Entity GetB(int id_B);
    public bool CancelB(int id_B, bool value);
    public bool ValideB(int id_B, bool value);

    // etc.. 
    //public int[] GetC_Pending();
    //public C_Entity GetC(int id_C);
    //public bool CancelC(int id_C, bool value);
    //public bool ValideC(int id_C, bool value);

}
public class A_Entity : ProcessableEntity
{
    public int A_EntityEntityDbId { get; set; }
    public string RealDataHere { get; set; }
}
public class B_Entity : ProcessableEntity
{
    public int B_EntityEntityDbId { get; set; }
    public string Rename { get; set; }
}

My question is : How to refactor this to avoid repeating the same process in Integrator_B, Integrator_C etc.. ?

What have I try: I went to the road of Func, Action, and delegate.
Giving codes like yhe following.

public bool Integration_Generic<T>(Func<List<T>> GetItems, Action<T[]> Integrator, string NoElementErreurMessage)
{
    var items = GetItems();
    if (!items.Any())
    {
        Logger.Log(this.GetType(), Level.Debug, $"Aucun {NoElementErreurMessage} à integrer.", null);
        return false;
    }
    Integrator(items.ToArray());
    return true;
}
private List<T> GetPending<T>(Func<int[]> PendingIds, Func<int, T> TGetter)
{
    var items = new List<T>();

    int[] itemsIds = null;
    try
    {
        itemsIds = PendingIds();
    }
    catch (Exception e)
    {
        Logger.Log(typeof(Integrateur),
            Level.Error,
            $"ERR {this.GetType().Namespace}.{PendingIds.Method.Name} : " + e
            , e);
    }

    if (itemsIds.Any())
    {
        Logger.Log(typeof(Integrateur),
            Level.Debug,
            $"{typeof(T).Name} itemsIds : [{string.Join(", ", itemsIds)}]"
            , null);
    }

    foreach (var id in itemsIds)
    {
        T tempT;
        try
        {
            tempT = TGetter(id);
            items.Add(tempT);
        }
        catch (Exception e)
        {
            Logger.Log(typeof(Integrateur),
                Level.Error,
                $"ERR {this.GetType().Namespace}.{TGetter.Method.Name}, Impossible de recuperer l'ID[{id}] : " + e
                , e);
        }
    }
    return items;
}

delegate V Creator_Delegate<T, U, V>(T input, out U output);

But there is too mutch compilation error for this code to be functional. Getting a fresh start may be better that fixing my try to tinker with things I don't fully understand.

6
  • You need to get rid of all the intermediate classes and refactor the service into ServiceA and ServiceB then you can have a single processing loop with a switch/generic/inheritance
    – Ewan
    Jan 4 '21 at 9:15
  • but you marked them as out of scope for editing
    – Ewan
    Jan 4 '21 at 9:18
  • @Ewan. Well those are ERP item. Those service are publish by the ERP. It's out of scope because it use it's own language x++ for Dynamics AX. Returned entites will never be the same. And those ERP object are Lovecraftian nigthmare. I can warp thing around them. That will refactor Cancel, Validate, and Get IDs.
    – Self
    Jan 4 '21 at 9:33
  • the trouble with wrapping is you need to wrap all the Response_A etc etc you just move your copy paste code down a layer
    – Ewan
    Jan 4 '21 at 9:38
  • "You will notice that the following code is 100% the same as A. With only a type difference." That's not correct. You are also calling different methods depending on that different type (e.g. ServiceERP.Submit_A versus ServiceERP.Submit_B). This significantly complicates any refactoring you do, as you'd be stuck trying to dynamically define access to different methods. Not saying it can't be done, but it's significantly more complex than if it was indeed only a different type being used.
    – Flater
    Jan 4 '21 at 10:06
1

Your code is in dire need of some SRP optimization. The responsibilities you're trying to mash into a single class (even in the current situation) should be distributed among several classes.

Your question is suggesting to start making use of generics, which would further complicate an already too complex class definition. Part of the reason why you're struggling to do this is because of the massive cognitive load it takes to understand this class in full.

However, the underlying question is a valid one. There is something that can be refactored for reusability here. But I'm not going to use your example. It's way too complex.

Let's start from a simplified example:

public class AService
{
    private readonly SourceService sourceService;
    private readonly ErpService erpService;

    public AService(SourceService sourceService, ErpService erpService)
    {
        this.sourceService = sourceService;
        this.erpService = erpService;
    }

    public void MigrateA()
    {
        A[] items = sourceService.GetA();

        foreach(var item in items)
        {
            erpService.CreateA(item)
        }
    }
}

I hope you agree that this is, in spirit, the same as your class. It's just a lot easier to understand for the sake of example.


I went to the road of Func, Action, and delegate.

Funcs, Actions and delegate are neat little features, but they are at a high risk of adding a significant amount of complexity. They're definitely useful, but should be used sparingly.

In this case, simple inheritance (combined with generics) will already do the trick for you. There's no need to resort to anything fancier.

When you want to convert a class to be inheritable and reusable, first you have to separate out the parts that are reusable and the parts which are not. The base class will contain everything that's reusable. The rest of the logic, which is not reusable, should instead be given an abstract placeholder.

public abstract class BaseService<TItem>
{
    public void Migrate()
    {
        TItem[] items = GetItems();

        foreach(var item in items)
        {
            Create(item);
        }
    }

    protected abstract TItem[] GetItems();

    protected abstract void Create(TItem item);
}

Notice the following changes:

  • The class is now abstract. This means that external consumers cannot directly instantiate it. It can only be derived, and those derived classes can be instantiated (unless they're abstract as well).
  • Instead of using A, we're using TItem as a generic type. In the end, we'll use A, B or C as TItem.
  • The Migrate logic is reusable, so it's part of the base class.
  • The non-reusable logic has been abstracted away into the abstract GetItems and Create methods. They don't have an implementation yet, this is going to be filled in by the derived classes.
  • GetItems and Create are set to protected, so that the derived classes can access them (but no one else can).
  • Since we are not using the SourceService and ErpService anymore (due to the now abstract methods), we don't need to store them in the base class. If the derived class uses them, the derived class should store them. It's tempting to still put them in the base class when you know that all the derived classes will be using it, but it's not correct. If a class never references an object, it does not need to store a reference to that object.

Now we have a reusable generic service, whose migration logic can be reused across many different services. Let's look at how we implement this now:

public class AService : BaseService<A>
{
    private readonly SourceService sourceService;
    private readonly ErpService erpService;

    public AService(SourceService sourceService, ErpService erpService)
    {
        this.sourceService = sourceService;
        this.erpService = erpService;
    }

    protected override A[] GetItems()
    {
        return sourceService.GetA();
    }

    protected override void Create(A item)
    {
        erpService.CreateA(item);
    }  
}

Notice:

  • AService now inherits from BaseService<A>, so it has reused the existing migration logic
  • AService only contains logic that is specific to the migration of A objects and could not be reused for B or C.
  • AService only has one public method. It's the Migrate method that it inherited from BaseService<A>.
4
  • Read your question once. Looked for my flash card. I have one on SRP. with 5 book/article read Agile ppp, Clean code, head first design. For some reason, I fail to use all of those design pattern in real life code. That version is mutch cleaner that what I have. Can't upvote with my rep. I will let the topics sit for at least 1 week before accepting answer.
    – Self
    Jan 4 '21 at 12:39
  • one quick question: You will call ServiceA.Migrate, ServiceB.Migrate, one after an other or. have a list of type and use reflection to call it? Or did I miss a simple principle ^^
    – Self
    Jan 4 '21 at 12:42
  • @Self: Never use a pattern you don't understand, unless someone you trust had advised you to implement it. Unnecessary patterns are the bane of any developer doing maintenance or reading the code.
    – Flater
    Jan 4 '21 at 12:42
  • @Self: I didn't really delve into how AService (and the others) get used by consumers. It very much depends on what you're trying to do. Reflection is a bad idea unless specifically warranted. I suspect you're going to have a CompleteMigrationService which in turn calls all the A/B/C/D/... migration services.
    – Flater
    Jan 4 '21 at 12:44

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.