Nistor Marian is a senior software developer at Tecknoworks with an extensive experience in the development and implementation of software programs using various technologies.
Eliminating Code Smells Through the Use of Simple Refactorings. Part 3 - Design Changes.
This article is the third part of "Eliminating Code Smells Through the Use of Simple Refactorings". In the first two parts, we wrote about refactoring through simple changes and execution path changes. You can access these articles on our blog–Part 1 and Part 2.
In the present article, we give a few tips and examples on refactoring through design changes.
Design changes usually involve the whole design of the system and are extremely hard to implement. They appear as a need during the development of the software system when faced with adding a new functionality that would break existing functionalities. This can be translated into the fact that the system’s original design can no longer accommodate changes easily, or cannot accommodate them at all. It is not a sign of bad design, but instead they should be considered as important milestones for the project. The more complex the system the more milestones like these you will have. These milestones allow you to grow the code base and improve it. Most of the times and most of the programmers will completely ignore these milestones and try to implement hacks in order to accomplish their tasks, which in time results in a code base that is not only unclean but also very hard to maintain. They need to be carefully planned and implemented. Having an end-to-end testing suite is more than advisable for these refactorings.
Class that requires information pertaining to another class in order to do its job. Clear indication of single responsibility principle violation. A new class should be created that would encapsulate that information and both of the other classes should make use of it. The principle that is being broken in this case is called The Law of Demeter, which states that you should never have obj1.propObj2.prop1 in your code.
Bad Code
public class Employee{
public decimal Overtime { get; set; }
}
public class Manager{
public List<Employee> Employees { get; set; }
}
public class ReportOvertime{
public decimal RetrieveTotalOvertime(Manager manager){
decimal overtime = decimal.Zero;
for (int i = 0; i < manager.Employees.Count; i++){
overtime += manager.Employees[i].Overtime;
}
return overtime;
}
}
As we can see the ReportOvertime class requires the Overtime information from the Employee class, which is breaking the law of Demeter. In order to fix this we will create a new method inside the Manager class that returns this information.
Good Code
public class Employee{
public decimal Overtime { get; set; }
}
public class Manager{
public List<Employee> Employees { get; set; }
public decimal RetrieveEmployeesOvertimeTotal(){
decimal overtime = decimal.Zero;
for (int i = 0; i < Employees.Count; i++){
overtime += Employees[i].Overtime;
}
return overtime;
}
}
public class ReportOvertime{
public decimal RetrieveTotalOvertime(Manager manager){
return manager.RetrieveEmployeesOvertimeTotal();
}
}
Now this code correctly respects the law of Demeter and if somehow the structure changes for the Manager class, let’s say a new list of HardcoreWorkers gets added, it will not affect the ReportOvertime in any way, but it will only affect the method that calculates the overtime which is part of the Manager class anyway. The report code will work like before, and we will correctly maintain the visibility of our hierarchical structure on an only to whom it concerns note.
“One switch rule” being broken. You have the same if or switch statement throughout different parts of the code. This usually calls forth the factory pattern which should bring the if or switch statement into one place where it can be managed in a much easier way.
Bad Code
public enum ShapeType{
Square,
Circle,
Cube,
Sphere
}
public class Shape{
public ShapeType Type { get; set; }
public double Width { get; set; }
public double Height { get; set; }
public double Radius { get; set; }
public Shape(ShapeType type, double width, double radius){
Type = type;
Width = width;
Radius = radius;
}
public double CalculatePerimeter(){
switch (Type){
case ShapeType.Square:
return 4 * Width;
case ShapeType.Circle:
return 2 * Math.PI * Radius;
case ShapeType.Cube:
return 12 * Width;
case ShapeType.Sphere:
return 4 * Math.PI * Radius;
default:
return double.NaN;
}
}
public double CalculateArea(){
switch (Type){
case ShapeType.Square:
return Width * Width;
case ShapeType.Circle:
return Math.PI * Math.Pow(Radius, 2);
case ShapeType.Cube:
return Math.Pow(Width, 3);
case ShapeType.Sphere:
return 4 * Math.PI * Math.Pow(Radius, 2);
default:
return double.NaN;
}
}
}
This is a classic scenario where you want to compute the perimeter or area of certain geometric shapes. As we can see the switch statement is being repeated in both methods. If we were to add a new type of Shape, we would have to be careful to add it in both places. If we were to add a new method to calculate volume, we would have to replicate the switch. This is not a very good code. In order to fix it we will add the factory pattern to create objects that derive from shape and that specifically handle one type of Shape.
Good Code
public enum ShapeType{
Square,
Circle,
Cube,
Sphere
}
public abstract class Shape{
protected Shape(ShapeType type, double width, double radius){
Type = type;
Width = width;
Radius = radius;
}
public ShapeType Type { get; set; }
public double Width { get; set; }
public double Radius { get; set; }
public abstract double CalculatePerimeter();
public abstract double CalculateArea();
public static Shape CreateAbstractShape(ShapeType type, double width, double radius){
switch (type){
case ShapeType.Square:
return new SquareShape(width);
case ShapeType.Circle:
return new CircleShape(radius);
case ShapeType.Cube:
return new CubeShape(width);
case ShapeType.Sphere:
return new SphereShape(radius);
default:
return null;
}
}
}
public class SquareShape : Shape{
public SquareShape(double width) : base(ShapeType.Square, width, double.NaN){
}
public override double CalculateArea(){
return Width * Width;
}
public override double CalculatePerimeter(){
return 4 * Width;
}
}
public class CircleShape : Shape{
public CircleShape(double radius) : base(ShapeType.Circle, double.NaN, radius){
}
public override double CalculateArea(){
return Math.PI * Math.Pow(Radius, 2);
}
public override double CalculatePerimeter(){
return 2 * Math.PI * Radius;
}
}
public class CubeShape : Shape{
public CubeShape(double width) : base(ShapeType.Cube, width, double.NaN{
}
public override double CalculateArea(){
return Math.Pow(Width, 3);
}
public override double CalculatePerimeter(){
return 12 * Width;
}
}
public class SphereShape : Shape{
public SphereShape(double radius) : base(ShapeType.Sphere, double.NaN, radius){
}
public override double CalculateArea(){
return 4 * Math.PI * Math.Pow(Radius, 2);
}
public override double CalculatePerimeter(){
return 4 * Math.PI * Radius;
}
}
As you can see four new classes have been created that all inherit the abstract class Shape. Each of them have their own ways of calculating the area and the perimeter. The switch statement was moved into the CreateAbstractShape static method. The constructors of the four classes have only the parameters they actually need and further refactoring can be done to simplify the Shape class constructor as well. Another important thing to actually notice is the fact that the Shape class is marked as abstract meaning we can no longer create instances of it, but rather we are forced to use the factory method.
A function or method that does more than what its name says. Usually it's a long method with lots and lots of if statements. Most of the time even the name will point towards the fact that the method does more than it should. We refactor this by splitting the method into smaller methods or functions.
Bad Code
public class Employee{
public DateTime StartDate { get; set; }
public DateTime EndDate { get; set; }
public decimal Hours { get; set; }
public decimal Overtime { get; set; }
public decimal Salary { get; set; }
}
public class Accountant{
private FinanceEngine Engine { get; set; }
public List<Employee> ManagedEmployees { get; set; }
public void CalculateSalaries(){
foreach (var emp in ManagedEmployees){
if (emp.EndDate <= DateTime.Today)
continue;
var normalSalary = emp.Hours * 5m; // normal hour rate of 5
var overtimeSalary = emp.Overtime * 10m; // overtime hour rate of 10
var salary = (normalSalary + overtimeSalary) * 1.5m; // add the taxes
emp.Salary = salary;
Engine.GenerateStatementToPrint();
}
}
}
The method in question is CalculateSalaries. Besides doing what the method name implies it also generates a statement that will eventually get printed. Also take a good look at the fact that employees get filtered in the method based on EndDate. Now in order to fix this code we will split this method into more specific methods and also rename this method in order to provide better explanation of what it actually does.
Good Code
public class Employee{
public DateTime StartDate { get; set; }
public DateTime EndDate { get; set; }
public decimal Hours { get; set; }
public decimal Overtime { get; set; }
public decimal Salary { get; set; }
}
public class Accountant{
private FinanceEngine Engine { get; set; }
public List<Employee> ManagedEmployees { get; set; }
public void ProcessSalaries(){
foreach (var emp in GetActiveEmployees()){
CalculateEmployeeSalary(emp);
Engine.GenerateStatementToPrint(emp);
}
}
private List<Employee> GetActiveEmployees(){
return ManagedEmployees.Where(x => x.EndDate > DateTime.Today).ToList();
}
private void CalculateEmployeeSalary(Employee emp){
var normalSalary = CalculateNormalHoursPart(emp);
var overtimeSalary = CalculateOvertimeHoursPart(emp);
var salary = AddTaxesToSalary(normalSalary + overtimeSalary);
emp.Salary = salary;
}
private decimal CalculateNormalHoursPart(Employee emp){
return emp.Hours * 5m; // normal hour rate of 5
}
private decimal CalculateOvertimeHoursPart(Employee emp){
return emp.Overtime * 10m; // overtime hour rate of 10
}
private decimal AddTaxesToSalary(decimal salary){
return salary * 1.5m;
}
}
First of all, we renamed the method name to ProcessSalaries in order to convey the fact that we are not only calculating the salary but also generate the statement to print. Then we moved the filtering of employees to a separate method. Everything else is pretty much obvious. We tried to isolate as much of the business logic behind the calculation of a salary into smaller methods to further minimize the impact that a change would have. We could further get rid of all those magic numbers through the use of constants. The most important thing to notice here is the fact that I can read this code top to bottom, as all the private method that a method uses are right below it, in order of usage.
Conclusion
Writing clean code is an art in itself, but turning already existing code into clean code is more of an experience matter than art! The whole idea behind it is that we already have the knowledge and the tools to do what’s right, but sometimes we choose to take the easy way out. In order to write awesome code, you do not need to be an artist, you do not need that skill, but you need to learn some of the basics principles of programming. Learning them will not only enhance your experience but will also provide you with the vision that is required in order to understand code and to mold it into that clean code that everybody wants to have and to work on. Due to this refactoring is not an art but rather a self-imposed discipline that you can follow no matter your skill or knowledge. No one will ever make you write the Mona Lisa in code, but through a series of refactorings you can end up with the same beauty in your code.
comments powered by Disqus