Low Coupling by Refactoring Towards Higher Cohesion
Low coupling and high cohesion go hand in hand. In a low coupled design, the modules, classes and functions have a high cohesion. The other way around is also true: making high cohesive modules, classes or functions leads to a loosely coupled design.
Why is this useful or interesting? The thing is that in designing software we never get directly to a good design. We can’t make a good design from the start. We improve the design of our code by gradually refactoring it. We put into the code the knowledge we learn by experimenting with it. This is code refactoring. Therefore, I think it is important to identify and to know patterns of refactoring, in order to increase our efficiency in getting to a good design. Refactoring towards high cohesive classes and functions is a pattern of refactoring our code to result a lower coupled design. I will present it bellow by walking through an example.
This article turned out to be quite long. For easier reading I have splat it in three. The first part reviews few concepts and the second part dives into the code, walks us through the small refactoring steps and explains the reasoning behind. At the end it summaries the refactoring pattern steps and a few smells to pay attention to.
Part 1: reviewing few concepts
Coupling refers to how tight the connection is between two elements of our system. Lower the coupling is the better isolated the elements of our system are. This means that changes in one element would not trigger changes in the others. Generally, the looser the coupling is the better the design is because it is less resistant to changes. However, some coupling is needed. The system elements do need to interact, and in a loosely coupled design this interaction is well defined through abstractions. The inner details of an element (module or class) are well encapsulated (hidden) and are not relevant when thinking on how to interact with others. When details change, they do not trigger waves of changes in all the system making it resistant to change. So coupling is tolerated where it is needed, meaning when it brings benefits and it should be done through good abstractions. In other words: Don’t tolerate coupling without benefits!
Identifying the good abstractions and implementing them through well encapsulated elements is not trivial, because we have a limited view on the system in the beginning. We couple things by nature. We put things together when we do not see the whole picture. Then when it is revealed, we need to stop, we need to refactor to break them up. Then we can identify the good abstractions which lead towards a better design.
A class has high cohesion when most of (all off) its fields are used by most of (all of) its methods. The more fields a method manipulates the more cohesive that method is to the class. A class in which each field is used by each method is maximally cohesive. When cohesion is high, the methods and fields of the class are co-dependent and hang together as a logical whole. Generally, it is neither advisable nor possible to create such maximally cohesive classes, but on the other hand we would like the cohesion to be high. For a function, cohesion refers to how closely the operations in a routine are related. The function code should consistently use all the variables it declare and all the input parameters to compute the result.
When we observe a class with poor cohesion, for example a class which has two parts, one made of three public methods which operate on two fields and another one made of two public methods which operate on other three fields, the general pattern to increase its cohesion is to split it in two classes and to make one to use the other. This way we can identify good abstractions and more code reuse opportunities, which will improve our design.
The example we will dive into shows a pattern to refactor to lower coupling, by improving the cohesion. The first step is to improve the public interface of a class by reducing the number of parameters of its functions. This reveals better the low cohesion of the class and then in successive refactoring steps we increase it, by splitting the class into more.
The pattern to reduce the number of parameters from public methods is to transform them into class fields, which are set through the constructor. This makes the class interface to be more cohesive and it reduces the complexity of the caller code. As consequence, it may also reveal the poor cohesion of the class, by now having groups of fields used by groups of methods. The refactor goes in small steps improving the design a little by little in controllable small chunks.
Part 2: refactoring example
The class that we are looking at is a class that is used to export XML files that represent pages of data about customers and their orders. The class gives functions that can export XML pages with detailed customer information or only with orders. The core functionality this class implements stays in the logic to fill in a XML with data (to simplify the example I have taken out this code)
To follow easier this refactoring example, you can download the source code file from each refactoring step from github here. We start with 00_PageXmlExport.cs, which is the initial state of the code and then when we advance you can look at 01_PageXmlExport.cs after first refactor step, then 02_PageXmlExport.cs for the second refactor step and so on until the fifth refactor step.
// file: http://tinyurl.com/00-PageXmlExport-cs
public class PageXmlExport
{
private const string exportFolder = "c:temp";
public bool ExportCustomerPage(
string fileNameFormat,
bool overwrite,
string customerName,
int maxSalesOrders,
bool addCustomerDetails)
{
string fileName = string.Format(fileNameFormat, "CustomerPage", customerName, DateTime.Now);
string filePath = Path.Combine(exportFolder, fileName);
if (!overwrite && File.Exists(filePath))
return false;
PageXml content = new PageXml {Customer = new CustomerXml {Name = customerName}};
using (var repository = new EfRepository())
{
if (maxSalesOrders > 0)
{
var orders = repository.GetEntities<Order>()
.Where(o => o.Customer.CompanyName == customerName)
.OrderBy(o => o.OrderDate)
.Take(maxSalesOrders);
//enrich content with orders
// …
}
if (addCustomerDetails)
{
var customer = repository.GetEntities<Customer>()
.Where(c => c.CompanyName == customerName);
// enrich content with customer data
// …
}
}
XmlSerializer serializer = new XmlSerializer(typeof (PageXml));
using (StreamWriter sw = File.CreateText(filePath))
{
serializer.Serialize(sw, content);
}
return true;
}
…
}
ExportCustomerPage(…) function will write on the disk an XML page containing customer details and customer orders, which are read from the database using a Repository implementation. It receives as input parameters a fileNameFormat with the pattern to generate the name of the file and a flag that says whether it should overwrite the file in case it already exits. It also receives the name of the customer to export the data of, and the maximum orders that should be exported. It gets an extra setting weather it should include or not the customer details.
This function evolved over time in this shape. The desire to reuse some of the code that implements the logic of enriching the XML with orders into the one that also enriches the XML with customer data, made the developers to add a the addCustomerDetails flag and correspondent code in here. Also the logic of composing the file name and/or overwriting it was added later, in the easiest way to implement a new request.
Looking further, the same desire to reuse the code that enriches the XML, made it that later a new function which includes external data into the exported XML was added to the same class. This is how ExportCustomerPageWithExternalData(…) got written. It does the same as the above, but if external data is present, then it is included in the XML.
// file: http://tinyurl.com/00-PageXmlExport-cs
public class PageXmlExport
{
…
public bool ExportCustomerPageWithExternalData(
string fileNameFormat,
bool overwrite,
string customerName,
int maxSalesOrders,
bool addCustomerDetails,
PageData externalData,
ICrmService crmService,
ILocationService locationService)
{
string fileName = string.Format(fileNameFormat, "CustomerPage", customerName, DateTime.Now);
string filePath = Path.Combine(exportFolder, fileName);
if (!overwrite && File.Exists(filePath))
return false;
PageXml content = new PageXml {Customer = new CustomerXml {Name = customerName}};
if (externalData.CustomerData != null)
{
// enrich content with externalData.CustomerData
// …
}
else
{
CustomerInfo customerData = crmService.GetCustomerInfo(
content.Customer.Name);
// enrich content with customer data
// …
}
using (EfRepository repository = new EfRepository())
{
if (maxSalesOrders > 0)
{
var orders = repository.GetEntities<Order>()
.Where(o => o.Customer.CompanyName == content.Customer.Name)
.OrderBy(o => o.OrderDate)
.Take(maxSalesOrders);
//enrich content with orders
}
if (addCustomerDetails)
{
var customer = repository.GetEntities<Customer>()
.Where(c => c.CompanyName == customerName);
// enrich content by merging the external customer data with what read from DB
// …
}
}
if (locationService != null)
{
foreach (var address in content.Customer.Addresses)
{
Coordinates coordinates = locationService.GetCoordinates(address.City, address.Street, address.Number);
if (coordinates != null)
address.Coordinates = string.Format("{0},{1}", coordinates.Latitude, coordinates.Longitude);
}
}
XmlSerializer serializer = new XmlSerializer(typeof (PageXml));
using (StreamWriter sw = File.CreateText(filePath))
{
serializer.Serialize(sw, content);
}
return true;
}
…
}
Following the same approach to reuse the core code that enriches the XML, other functions we added in time. ExportOrders(..) exports a XML with the same schema, but with all the orders the customer has and without additional customer data.
// file: http://tinyurl.com/00-PageXmlExport-cs
public class PageXmlExport
{
…
public bool ExportOrders(string fileNameFormat, bool overwrite, string customerName)
{
string fileName = string.Format(fileNameFormat, "CustomerOrdersPage", customerName, DateTime.Now);
string filePath = Path.Combine(exportFolder, fileName);
if (!overwrite && File.Exists(filePath))
return false;
PageXml content = new PageXml {Customer = new CustomerXml {Name = customerName}};
using (EfRepository repository = new EfRepository())
{
var orders = repository.GetEntities<Order>()
.Where(o => o.Customer.CompanyName == content.Customer.Name)
.OrderBy(o => o.ApprovedAmmount)
.ThenBy(o => o.OrderDate);
//enrich content with orders
}
XmlSerializer serializer = new XmlSerializer(typeof (PageXml));
using (StreamWriter sw = File.CreateText(filePath))
{
serializer.Serialize(sw, content);
}
return true;
}
…
}
Later on, because this was the class that knew how to produce XMLs with customer orders, it got two new methods: GetPagesFromOrders(…) and ExportPagesFromOrders(…), which were useful when the data should not be taken from the database, but it is given as input parameter.
// file: http://tinyurl.com/00-PageXmlExport-cs
public class PageXmlExport
{
…
public IEnumerable<PageXml> GetPagesFromOrders(
IEnumerable<Order> orders,
int maxSalesOrders,
ICrmService crmService,
ILocationService locationService)
{
Dictionary<string, IEnumerable<Order>> customerOrders = GroupOrdersByCustomer(orders);
foreach (var customerName in customerOrders.Keys)
{
PageXml content = new PageXml {Customer = new CustomerXml {Name = customerName}};
if (crmService != null)
{
CustomerInfo customerData = crmService.GetCustomerInfo(content.Customer.Name);
//enrich with data from crm
}
var recentOrders = customerOrders[customerName]
.OrderBy(o => o.OrderDate)
.Take(maxSalesOrders);
foreach (var order in recentOrders)
{
// enrich content with orders
// …
}
if (locationService != null)
{
foreach (var address in content.Customer.Addresses)
{
Coordinates coordinates = locationService.GetCoordinates(address.City, address.Street, address.Number);
if (coordinates != null)
address.Coordinates = string.Format("{0},{1}", coordinates.Latitude, coordinates.Longitude);
}
}
yield return content;
}
}
public bool ExportPagesFromOrders(
string fileNameFormat,
bool overwrite,
IEnumerable<Order> orders,
int maxSalesOrders,
ICrmService crmService,
ILocationService locationService)
{
IEnumerable<PageXml> pages = GetPagesFromOrders(orders, maxSalesOrders, crmService, locationService);
foreach (var pageXml in pages)
{
string customerName = pageXml.Customer.Name;
string fileName = string.Format(fileNameFormat, "CustomerOrdersPage", customerName, DateTime.Now);
string filePath = Path.Combine(exportFolder, fileName);
if (!overwrite && File.Exists(filePath))
return false;
XmlSerializer serializer = new XmlSerializer(typeof (PageXml));
using (StreamWriter sw = File.CreateText(filePath))
{
serializer.Serialize(sw, pageXml);
}
}
return true;
}
…
}
Now, look at the public methods signature only. They have many and redundant parameters. This makes them hard to be used by the caller code.
// file: http://tinyurl.com/00-PageXmlExport-cs
public class PageXmlExport
{
…
public bool ExportCustomerPage(
string fileNameFormat,
bool overwrite,
string customerName,
int maxSalesOrders,
bool addCustomerDetails)
{…}
public bool ExportCustomerPageWithExternalData(
string fileNameFormat,
bool overwrite,
string customerName,
int maxSalesOrders,
bool addCustomerDetails,
PageData externalData,
ICrmService crmService,
ILocationService locationService)
{…}
public bool ExportOrders(
string fileNameFormat,
bool overwrite,
string customerName)
{…}
public IEnumerable<PageXml> GetPagesFromOrders(
IEnumerable<Order> orders,
int maxSalesOrders,
ICrmService crmService,
ILocationService locationService)
{…}
public bool ExportPagesFromOrders(
string fileNameFormat,
bool overwrite,
IEnumerable<Order> orders,
int maxSalesOrders,
ICrmService crmService,
ILocationService locationService)
{…}
…
}
The first step of refactor is to reduce the number of parameters of the first two methods. We move some common parameters into the constructor. It results the following code (file 01_PageXmlExport.cs).
// file: http://tinyurl.com/01-PageXmlExport-cs
public class PageXmlExport
{
private const string exportFolder = "c:temp";
private readonly string fileNameFormat; //used in 3/5 methods
private readonly bool overwrite; //used in 3/5 methods
private readonly int maxSalesOrders; // used in 3/5 methods
private readonly bool addCustomerDetails; // used in 2/5 methods
public PageXmlExport(string fileNameFormat,
bool overwrite,
int maxSalesOrders,
bool addCustomerDetails)
{
this.fileNameFormat = fileNameFormat;
this.overwrite = overwrite;
this.maxSalesOrders = maxSalesOrders;
this.addCustomerDetails = addCustomerDetails;
}
public bool ExportCustomerPage(string customerName)
{…}
public bool ExportCustomerPageWithExternalData(
string customerName,
PageData externalData,
ICrmService crmService,
ILocationService locationService)
{…}
…
}
This already makes the functions to look better. They are easier to be called. The caller can now instantiate this class with the settings it needs, and then call ExportCustomerPage(…) for all the customer names it wants. Previously, it had to repeat most of the parameters for each customer call. If we look at the resulted fields we can see that most of them are used in 3 of the 5 functions. The rest of the code remains pretty much the same.
Continuing on this path, the next small refactor step is to reduce the number of parameters for the next functions. The resulted code is the following (file 02_PageXmlExport.cs).
// file: http://tinyurl.com/02-PageXmlExport-cs
public class PageXmlExport
{
private const string exportFolder = "c:temp";
private readonly string fileNameFormat; //used in 3/5 methods
private readonly bool overwrite; //used in 3/5 methods
private readonly int maxSalesOrders; // used in 3/5 methods
private readonly bool addCustomerDetails; // used in 2/5 methods
private readonly ICrmService crmService; // used in 3/5 methods
private readonly ILocationService locationService; // used in 3/5 methods
public PageXmlExport( string fileNameFormat,
bool overwrite,
int maxSalesOrders,
bool addCustomerDetails,
ICrmService crmService,
ILocationService locationService)
{ … }
public bool ExportCustomerPageWithExternalData(
string customerName,
PageData externalData)
{ … }
public bool ExportOrders(string customerName)
{ … }
public IEnumerable<PageXml> GetPagesFromOrders(IEnumerable<Order> orders)
{ … }
public bool ExportPagesFromOrders(IEnumerable<Order> orders)
{ … }
…
}
Now looking again only at the signature of the public functions of this class. They look much better. The public interface is more consistent and this simplifies the caller code.
Analyzing further the refactor result, we see that the class has in the constructor 6 dependencies. This is quite a lot. Looking at the fields, it has 7 fields, from which 2 are not complex (ICrmService and ILocationService). Having so many fields and dependencies is a bad smell. On a closer look we may observe that some fields are used only by some functions and not all functions use all fields. This is a clear symptom of a class with poor cohesion.
To refactor towards a better cohesive class, we should identify groups of fields, used by same methods. For example, the exportFolder, fileNameFormat and overwrite are used by all functions that write to disk. Even more, they are only used in the logic that relates to writing the file. When we identify such a group of dependencies it is an opportunity to reduce the dependencies of the class by moving them into a new class, which will be used by the current one. While doing this, is good to also think about a good abstraction for the class we are going to create, an abstraction that express well the functionality the new class is going to provide. For our example we will have it as an interface which has a function that writes a PageXml. If it is hard to come up with a good abstraction, we can postpone this and do it in two steps: first split the classes and then find a good abstraction. Doing this refactor step it results the following code (file 03_PageXmlExport.cs).
// file: http://tinyurl.com/03-PageXmlExport-cs
public interface IPageFileWriter
{
bool WriteFile(PageXml page, string filePrefix);
}
public class PageXmlExport
{
private readonly IPageFileWriter fileWriter;
private readonly int maxSalesOrders; // used in 3/5 methods
private readonly bool addCustomerDetails; // used in 2/5 methods
private readonly ICrmService crmService; // used in 3/5 methods
private readonly ILocationService locationService; // used in 3/5 methods
public PageXmlExport( IPageFileWriter fileWriter,
int maxSalesOrders,
bool addCustomerDetails,
ICrmService crmService,
ILocationService locationService)
{ … }
public bool ExportCustomerPage(string customerName)
{
PageXml content = new PageXml {Customer = new CustomerXml {Name = customerName}};
using (EfRepository repository = new EfRepository())
{
if (maxSalesOrders > 0)
{
var orders = repository.GetEntities<Order>()
.Where(o => o.Customer.CompanyName == content.Customer.Name)
.OrderBy(o => o.OrderDate)
.Take(maxSalesOrders);
//enrich content with orders
// …
}
if (addCustomerDetails)
{
var customer = repository.GetEntities<Customer>()
.Where(c => c.CompanyName == customerName);
// enrich content with customer data
// …
}
}
return fileWriter.WriteFile(content, "CustomerPage");
}
…
}
This refactor step has reduced the number of fields by groping three of them into one. As a consequence it also reduced the dependencies of the class and it made all its methods better by taking out the code that deals with the details of writing a file.
Now, is the moment to think about a better abstraction for the interface we have created: the IPageFileWriter. We could make it more abstract if we rename it and take out or at least rename the filePrefix parameter of its method. The interface in its form limits its implementation to writing to files. A better one would be:
public interface IPageWriter
{
bool Write(PageXml page);
}
This interface may have implementations that could write the XML anywhere. By increasing this abstraction we are increasing the reusability of the PageXmlExport class, by being able to reuse the same code and logic to export in multiple mediums. We are not going to pursue this refactoring path, but we are going to continue with increasing the cohesion of the PageXmlExport.
Lets see other groups of fields that could be extracted. Looking at maxSalesOrders or addCustomerDetails it is hard to see with which to group them. On the other hand we observe that crmService and locationService are used by the functions that enrich the XML with external data. Therefore one idea is to group these two together in a new depenedency. Because it gives data for the export class we name it as IExportDataProvider interface. It results the following code (file 04_PageXmlExport.cs)
// file: http://tinyurl.com/04-PageXmlExport-cs
public interface IExportDataProvider
{
CustomerInfo GetCustomerInfo(string name);
Coordinates GetCoordinates(string city, string street, string number);
}
public class PageXmlExport
{
private readonly IPageFileWriter fileWriter;
private readonly IExportDataProvider dataProvider;
private readonly int maxSalesOrders; // used in 4/5 methods
private readonly bool addCustomerDetails; // used in 2/5 methods
public PageXmlExport( IPageFileWriter fileWriter,
int maxSalesOrders,
bool addCustomerDetails,
IExportDataProvider dataProvider)
{ … }
…
}
The number of fields and dependences got smaller after this step. However, the code of the class did not improve much. It is the same, but instead calling functions from ICrmService respectively ILocationService, now all the calls go to the same IExportDataProvider. IExportDataProvider implementations will wrap together the external services for the export class, but something doesn’t smell good about it. The first bad signal comes from the name. It is named data provider, but it also gets geo location coordinates. Looking at it from another perspective, we can also see that the interface mixes different levels of abstraction. The CustomerInfo is a high level concept in comparison with Coordinates.
Looking again at the resulted code in PageXmlExport class, we see that it now has this IExportDataProvider, but it also uses a repository to get data from the database. Maybe a better idea would have been to wrap also the repository in the IExportDataProvider. The repository is another dependency our class has, but it is not that visible. The refactoring that we have done made us think to the dependencies more, and now with the IExportDataProvider as a visible dependency in the constructor and the repository being used in most of the methods, it is much more obvious that they should be grouped together as one dependency.
Lets rollback the last refactor step that we did, and wrap the ICrmService together with the repository in a real IExportDataProvider. Thinking on doing this we also observe that the other dependencies our class has (maxSalesOrders and addCustomerDetails settings) are only used together with the hidden dependency towards the repository. They all could be wrapped into the IExportDataProvider. The IExportDataProvider implementations should depend on these settings and them should instruct the repository accordingly. In fact, by doing this we are taking out from the PageXmlExport class the responsibility of being setup about how to read data and we push that lower to IExportDataProvider implementations. The resulted code after this refactor step is the following (file 05_PageXmlExport.cs).
// file http://tinyurl.com/05-PageXmlExport-cs
public class PageXmlExport
{
private readonly IPageFileWriter fileWriter;
private readonly IExportDataProvider_5 dataProvider;
private readonly ILocationService locationService;
public PageXmlExport( IPageFileWriter fileWriter,
IExportDataProvider dataProvider,
ILocationService locationService)
{
this.fileWriter = fileWriter;
this.dataProvider = dataProvider;
this.locationService = locationService;
}
public bool ExportCustomerPage(string customerName)
{
PageXml content = new PageXml {Customer = new CustomerXml {Name = customerName}};
IEnumerable<CustomerData> orders = dataProvider.GetCustomerOrders(customerName);
// enrich content with orders
// ..
// enrich content with customer data
// ..
return fileWriter.WriteFile(content, "CustomerPage");
}
public bool ExportCustomerPageWithExternalData(
string customerName,
PageData externalData)
{
PageXml content = new PageXml {Customer = new CustomerXml {Name = customerName}};
if (externalData.CustomerData != null)
{
// enrich with externalData.CustomerData
// …
}
else
{
CustomerInfo customerData =
dataProvider.GetCustomerInfo(content.Customer.Name);
}
IEnumerable<CustomerData> orders = dataProvider.GetCustomerOrders(customerName);
// enrich content with orders
// …
// enrich content by merging the external customer data with what read from DB
// …
foreach (var address in content.Customer.Addresses)
{
Coordinates coordinates = locationService.GetCoordinates(address.City, address.Street, address.Number) ;
if (coordinates != null)
address.Coordinates = string.Format("{0},{1}", coordinates.Latitude, coordinates.Longitude);
}
return fileWriter.WriteFile(content);
}
public bool ExportOrders(string customerName)
{
PageXml content = new PageXml {Customer = new CustomerXml {Name = customerName}};
IEnumerable<CustomerData> orders = dataProvider.GetCustomerOrders(customerName);
//enrich content with orders
return fileWriter.WriteFile(content);
}
…
}
Looking now at the result, we are definitely in a much better place than when we were when we started. Maybe our code design is not perfect nor the best, but it is definitely better. Now our PageXmlExport class has three dependencies, which are used by most of its functions. Most of the code in the class got smaller and simpler. The methods follow a simple pattern: they get data using the IExportDataProvider, they enrich the content XML with it (which is the core functionality and knowledge of our class), and they output the result. The details of reading data and output the result are not important. Our class can focus on XML enrichment only.
Maybe we do not yet have the best abstractions in place, but now after we have separated better the concerns it is a good moment to think more about abstraction and encapsulation. The code that gets data from the database or other sources is now decoupled from the core code that enriches the XML. We have achieved the same decoupling for the code that writes the XML on the disk. The details of how these are done are no longer mixed with XML enrichment logic and by abstracting them we can now reuse the XML enrichment code to read data from any medium and output the results to any medium. In conclusion we have achieved a better Separation of Concerns, and a more loosely coupled design, by following a simple refactoring pattern. We did this almost in a mechanical way looking to reduce the number of parameters or the number of dependencies.
Part 3: summarizing the refactoring pattern
If we recap the refactoring pattern we have followed to achieve loose coupling by improving coherence we have the following steps or recommendations:
- Be critical with functions that have more than about 4 parameters. It may be that they are part of poor cohesive classes or in they have a poor cohesion themselves.
- reduce the number of parameters of functions by transforming the common (redundant) ones into class fields received through the constructor.
- do this in small refactoring steps
- this may reveal a class with poor cohesion
- Be critical with classes that contain more than about 7±2 fields (more toward the high end of 7±2 if the fields are primitives and more toward the lower end of 7±2 if the fields are pointers to complex objects or services)
- the number 7±2 has been found to be a number of discrete items a person can remember while performing other tasks
- reduce the number of fields by grouping the ones that are used by same functions into one new field. This leads to taking out code, and it increases the coherence of the original class
- Be critical with classes that have more than about 4±1 dependencies - most probably they have poor cohesion
- most probably the dependencies may be reduced by grouping and extracting them in new classes. This increases the cohesion and leads to a loosely coupled code
- think about putting in place good abstractions for new extracted classes. This will increase the reusability and extensibility of the code.
- think about different levels of abstractions and different levels reasons of change when you try to improve the abstractions that define the interaction between your classes
By following these with the above example we have refactored from a class that mixes code with details of creating and writing into files, with code that implements high level business logic (XML enrichment) and with code that deals with data access concerns, into a more decoupled design where all these concerns are separated and may be abstracted better.