Methods should do one thing only

According to the Single Responsibility Principle a class should have one, and only one, reason to change. To same statement is valid for methods. A method should do one thing only and therefore have only one reason to change.

Unfortunately this principle will be disregarded often. And I think the main reason is that it isn’t that easy to say whether a method does one or several things. Within this article I want to give you some hints which may help you to count the responsibilities of a method.

Does your method delegate or implement tasks?

In general, source code can contain code to execute a task or code which delegates the task execution to another method or class. For example you want to create a file with some content. You may use the File class of the .NET framework and use the open, write and close methods to implement you file handling task. This implementation contains execution code. Your source code implicitly implements the task. This implementation will normally be part of a method. Within another class this method is used. For example prior to closing your application you want to store all data. So your shutdown module will call the implemented file creation method. This shutdown method will therefore delegate the work to another method.

Methods which delegate the work to other methods will most often contain several of such delegations. They will call them by using a logical order or workflow. Therefore I normally call these two types of code: “execution code” and “logical code”. Execution code is the implementation of a task and logical code contains a workflow which uses the methods of the execution code.

In my opinion the separation between execution code and logical code is a base concept for clean software architecture. And this concept will help us to solve the issue of this article. Therefore I want to ask the following question: “Is a method which contains execution code and logical code able to do one thing only or will it always do several things?”

I think it will help to look at a little example. The following example shall implement a report generation. The report will contain the salary for all employees. To make it easy we will not implement every detail. Let’s say we have following components: a data class for an employee and a report component which can create pages and add some headers and text elements. Furthermore it is possible to add a new page be defining the content.

interface IReportContent
{
	//...
}

interface IReport
{
	void CreateNewPage();
	void AddHeader(string header);
	void AddText(string content);
	void AddPage(IReportContent content);
}

interface IEmployee
{
	string Name { get; }
	double Salary { get; }
}

Or report shall contain a page with some base information, pages for all employees and a summary page. Therefore we will implement the following method.

class ReportGenerator
{
	public IReport CreateSalaryReport(List<IEmployee> employees)
	{
		IReport report = new Report();
					
		//create header
		report.CreateNewPage();
		report.AddHeader("Employee Report");
		report.AddText("Date: " + DateTime.Now);
		report.AddText("Created by: " + "...");

		//add all employees
		foreach (IEmployee employee in employees)
		{
			IReportContent content = GetEmployeeSalaryReportPage(employee);
			report.AddPage(content);
		}
			
		//add summary
		report.CreateNewPage();
		report.AddHeader("Total Pays:");
		report.AddText("....");
	}

	private IReportContent GetEmployeeSalaryReportPage(IEmployee employee)
	{
		//...
	}
}

What do you think if you look at this method? How many responsibilities does this method have? Or in the words of the Single Responsibility Principle: How many reasons for a change exist?

I think the method does three things. It creates the header page, it creates the summary page and it concatenates all pages to create the report. Or in other words, the method has three reasons for a change: A change of the content of the header page, a change of the content of the summary page or a change of the report structure.

Therefore we have to refactor this method. In this case you can extract the header page creation and the summary page creation into own methods.

class ReportGenerator
{
	public IReport CreateSalaryReport(List<IEmployee> employees)
	{
		IReport report = new Report();
		IReportContent content;

		content = GetReportHeader();
		report.AddPage(content);

		foreach (IEmployee employee in employees)
		{
			content = GetEmployeeSalaryReportPage(employee);
			report.AddPage(content);
		}

		content = GetReportSummary();
		report.AddPage(content);

	}

	private IReportContent GetReportHeader()
	{
		IReportContent content;
		content.AddHeader("Employee Report");
		content.AddText("Date: " + DateTime.Now);
		content.AddText("Created by: " + "...");

		return content;
	}

	private IReportContent GetEmployeeSalaryReportPage(IEmployee employee)
	{
		//...
	}

	private IReportContent GetReportSummary()
	{
		IReportContent content;
		content.AddText("Total Pays:");
		content.AddText("....");

		return content;
	}
}

With this small refactoring the responsibilities of the method have greatly changed. Now the method has one responsibility only to create a report by combining the report parts. As a result there is only one reason for a change: the page structure of the report changes, for example the summary page shall be removed.

After this example I want to come back to the topic and question of this chapter: “Does your method delegate or implement tasks?”

As you can see the first version of the method does both. It implements the creation of the header and summary pages and it delegates the creation of the employee salary page. This mix of execution code and logical code is a clear signal that the method does several things. In summary I want to make the following statement:

A method which contains execution code and logical code will always ever do several things. Therefore you should never mix execution code and logical code within a method.

Does your method contain several separated logical workflows?

If your method contains execution code or logical code only, you need another possibility to see whether it does several things. In this case you should discover the logical workflows within the method. If you can find more than one workflow or if you see a chance to split up the existing workflow into different parts, than there is a high probability your method does more than one thing.

I want to show a little example. Let’s say we have a settings engine which should store settings data. The data is stored as encrypted string. To make it easy we will look at the settings engine only and hide the details of the database component and the settings data component.

class SettingsEngine
{
	IDatabaseController _database;
	IConfigurationController _configuration;

	public void SaveSettings()
	{
		//connect or create database
		if(_database.Connect() == false)
		{
			_database.Create();
		}

		//get settings data as encrypted data
		string settings;
		settings = _configuration.GetEncryptedSettingsData();

		//write to database
		if(_database.WriteSettingsData(settings) == false)
		{
			throw new DatabaseWriteException(...);
		}
	}
}

The method contains logical code only. So far so good, but let us check the second criteria. Does the method contain several logical workflows? Unfortunately: yes! There are three workflows. At first we have the overall workflow to store the settings data, which is the main workflow of the method. Second we have the procedure to initialize the database by connect to the database or create a new one if necessary. And there is a small third workflow which tries to write the data and throw an exception if the data update fails. As a result of this design there are three possible reasons for a method change: The connections procedure could be changed, for example throw an error and don’t create the database. The data update could be changed, for example repeat the write step with additional rights and don’t throw an error. And the whole storage workflow can be changed for example by removing the connection step as this can be done by another service class.

In summary we have two separated workflows within the method workflow. We can therefore refactor this method by extracting these workflows.

class SettingsEngine
{
	IDatabaseController _database;
	IConfigurationController _configuration;

	public void SaveSettings()
	{
		InitializeDatabase();

		string settings;
		settings = _configuration.GetEncryptedSettingsData();

		WriteSettingsToDatabase(settings);
	}

	private void InitializeDatabase()
	{
		if (_database.Connect() == false)
		{
			_database.Create();
		}            
	}

	private void WriteSettingsToDatabase(string settings)
	{
		if (_database.WriteSettingsData(settings) == false)
		{
			throw new DatabaseWriteException(...);
		}
	}
}

Now the method contains no individual workflows. There is only the main workflow to store the settings. The method will do one thing only now and therefore there is only one reason the change the method.

In summary there I can give the following recommendation:

A method which contains several separated logical workflows will always ever do several things. Therefore you should never implement more than one workflow within one method.

Summary

A base software concept is: A method should do one thing only or in other words there should only one reason to change a method. But the source code of nearly every application will contain methods which violate this rule. One reason may be that it isn’t that easy to see whether your method does more than one thing or not. Within this article I have introduced two easy checks which you can use to identify methods with several responsibilities.

Werbung
Dieser Beitrag wurde unter .NET, C#, Clean Code veröffentlicht. Setze ein Lesezeichen auf den Permalink.

Kommentar verfassen

Trage deine Daten unten ein oder klicke ein Icon um dich einzuloggen:

WordPress.com-Logo

Du kommentierst mit deinem WordPress.com-Konto. Abmelden /  Ändern )

Facebook-Foto

Du kommentierst mit deinem Facebook-Konto. Abmelden /  Ändern )

Verbinde mit %s