Welcome to yet another code review that we did this week on a service refactor.

Each feedback will have three sections

  1. File - The file that need to undergo the change
  2. TODO - Suggested changes to the file
  3. A description for why the TODO list should be done. This section is purely for educational purposes.


Baklava Code

Thin software layers don’t add much value, especially when you have many such layers piled on each other. Each layer has to be pushed onto your mental stack as you dive into the code.

In our context what this means is it's ok to have many layers (as many as it's really needed), but don't do single atomic functionality in many layers. For example if there is a service to send emails the service should own the email templates as well. Ideally the Template provider should neither be in Notification service not be in the MemberNetworkService because it is owned by neither of them. But a logical direction would be to get the templates into the Notification Service for now and then to a separate Template Service later. (when it's out of control)

Bandwagon effect

We tend to replicate the same pattern for different objects though they are not needed. Unless an object can take different forms and phenomena it's likely that it doesn't need an interface and a stateful object.

The idea of interface and injecting a concrete type is associated with modifying behavior at runtime through polymorphism. In the case where the interface has only one derived symbol they should be modelled as singletons, because they are designed to perform a static operation. This also avoids unnecessary injection of objects to constructor.

Note : If you have to inject a different interface for testing purpose then it makes sense to have DI containers inject the objects.

Readability

Particularly with OOP languages, we spend a lot of time on objects IO than computation. I would say almost all the time. So it's very important that we handle assignments and object access. The above line should be split into multiple (or) at least 2 parts. 1) to get the object that gives the functionality 2) invoking the functionality of the object. This way you know whether a failure is with the accessor (or) the functionality. Most importantly readability.

Large Refactoring


During large refactoring of projects we tend to move files around, but also carry the dependencies along. At the end of the refactoring the structure of the project would have changed, but the dependency graph would continue to be the same. When doing refactoring (not adding any new functionality) the number of dependencies should either stay the same or get less, but not more. To me dependencies are Project references, nuget packages, Assembly references, Source sharing etc.,

There are lot of practical tips to achieve that

When refactoring is done, open a project and remove it references and see what fails. List the number of types that fail and see how they can be better accommodated. Do we need these types ? Should the function that uses the types need to be in the project ? (or) some other existing project.

Are you splitting functionality across projects. It's fine to have layers that does one thing, but is the atomic nature of a function broken into multiple layers. For example in case of sending an email,

  • It's ok to break the functionality into layers where one layer gather inputs to send the email and the other layer take these inputs and do the email sending functionality.
  • It's  ok to split the part where we gather input in one layer, prepare the template in another layer and send the email in another layer.
  • It's not ok for one layer to own the template, another layer to use the template and substitute values. Here you are splitting an atomic activity to discrete pieces, which will create coupling between them.

Are you using references just to resolve the parent references. This is a classical mistake, my favourite. Let's take the following example.

     Foo1.dll (.NET Assembly)
     class Foo
     {
       public Foo2 GetFoo2()
       {
         return new Foo2();
       }
     }
     Foo2.dll (.NET Assembly)
     public Foo2
     {
       public Foo2()
       {  }
     }
     Contracts.dll (.NET Assembly)
     class Contracts
     {
       public static Foo GetFoo()
       {
           return new Foo();
       }
     }

Now in the above case, let's say that Foo2 is an implementation detail and should not be accessed directly and should be accessed only via Contracts Factory, the following is what he need to do,

public void GetFoo()
{
   varfoo2 = Contracts.GetFoo().GetFoo2();
}

And to achieve the above, we add reference to the following assemblies, but we only need Contracts class and the Foo2 class and not the Foo class (but forced to)

  • Contracts.dll, Foo1.dll, Foo2.dll

In this case we can actually modify the Contracts.dll to be like the following

     Contracts.dll (.NET Assembly)
     class Contracts
     {
       public static Foo GetFoo()
       {
           return new Foo();
       }
       public static Foo2 GetFoo2()
       {
           Foo().GetFoo2();
       }
     }

In the above case, the caller don't even need to know that something like Foo exists. He can directly call GetFoo2() to avoid referring Foo1.dll at all.

With this i will end this post, hope you got useful information on reviewing code and refactoring