You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Copy file name to clipboardExpand all lines: snippets/kill-the-if-chain.md
+357-1Lines changed: 357 additions & 1 deletion
Original file line number
Diff line number
Diff line change
@@ -1,6 +1,6 @@
1
1
# Kill the if-chain
2
2
3
-
##[Vahid Najafi](https://github.com/vahidvdn)
3
+
The nested structure could be unrolled, as suggested by[Vahid Najafi](https://github.com/vahidvdn)
4
4
5
5
```js
6
6
function()
@@ -15,3 +15,359 @@ function()
15
15
16
16
}
17
17
```
18
+
19
+
Actually, I feel there is way more space for refactoring, but it all depends on the economic of the projects.
20
+
21
+
It is worth to discuss the smells and design flaws of that code, to set up the refactoring goals.
22
+
23
+
## Smells
24
+
I see these problems:
25
+
26
+
* The code violates some of the [SOLID principles][2]. It surely violates the [Open Closed Principle][3], as it is not possible to extend it without changing its code. E.g., adding a new operation would require adding a new `if`/`else` branch;
27
+
* It also violate the [Single Responsibility Principle][4]. It just does too much. It performs error checks, it's responsible to execute all the 4 operations, it contains their implementations, it's responsible to check their results and to chain their execution in the right order;
28
+
* It violates the [Dependency Inversion Principle][5], because there are dependencies between high-level and low-level components;
29
+
* It has a horrible [Cyclomatic complexity][6]
30
+
* It exhibits high coupling and low cohesion, which is exactly the opposite of [what is recommended][7];
31
+
* It contains a lot of code duplication: the function `Succeeded()` is repeated in each branch; the structure of `if`/`else`s is replicated over and over; the assignment of `error` is duplicated.
32
+
* It could have a pure functional nature, but it relies instead on state mutation, which makes reasoning about it not easy.
33
+
* There's an empty `if` statement body, which might be confusing.
34
+
35
+
## Refactoring
36
+
Let's see what could be done.<br/>
37
+
Here I'm using a C# implementation, but similar steps can be performed with whatever language.<br/>
38
+
I renamed some of the elements, as I believe honoring a naming convention is part of the refactoring.
39
+
40
+
```csharp
41
+
internal classTestClass
42
+
{
43
+
HResult SomeFunction()
44
+
{
45
+
var error =HResult.Ok;
46
+
47
+
if(Succeeded(Operation1()))
48
+
v {
49
+
if(Succeeded(Operation2()))
50
+
{
51
+
if(Succeeded(Operation3()))
52
+
{
53
+
if(Succeeded(Operation4()))
54
+
{
55
+
}
56
+
else
57
+
{
58
+
error =HResult.Operation4Failed;
59
+
}
60
+
}
61
+
else
62
+
{
63
+
error =HResult.Operation3Failed;
64
+
}
65
+
}
66
+
else
67
+
{
68
+
error =HResult.Operation2Failed;
69
+
}
70
+
}
71
+
else
72
+
{
73
+
error =HResult.Operation1Failed;
74
+
}
75
+
76
+
return error;
77
+
}
78
+
79
+
private string Operation1()
80
+
{
81
+
// some operations
82
+
return"operation1 result";
83
+
}
84
+
private string Operation2()
85
+
{
86
+
// some operations
87
+
return"operation2 result";
88
+
}
89
+
private string Operation3()
90
+
{
91
+
// some operations
92
+
return"operation3 result";
93
+
}
94
+
private string Operation4()
95
+
{
96
+
// some operations
97
+
return"operation4 result";
98
+
}
99
+
100
+
private bool Succeeded(string operationResult) =>
101
+
operationResult =="some condition";
102
+
}
103
+
104
+
internal enum HResult
105
+
{
106
+
Ok,
107
+
Operation1Failed,
108
+
Operation2Failed,
109
+
Operation3Failed,
110
+
Operation4Failed,
111
+
}
112
+
}
113
+
```
114
+
115
+
For the sake of simplicity, I supposed each operation returns a string, and that the success or failure is based on an equality check on the string, but of course it could be whatever. In the next steps, it would be nice if the code is independent from the result validation logic.
116
+
117
+
### Step 1
118
+
It would be nice to start the refactoring with the support of some test harness.
Our case is a trivial one, but being the quiz supposed to be a job interview question, I would not ignore it.
141
+
142
+
143
+
### Step 2
144
+
The first refactoring could be getting rid of the mutable state: each if branch could just return the value, instead of mutating the variable `error`. Also, the name `error` is misleading, as it includes the success case. Let's just get rid of it:
145
+
146
+
```csharp
147
+
HResult SomeFunction()
148
+
{
149
+
if(Succeeded(Operation1()))
150
+
{
151
+
if(Succeeded(Operation2()))
152
+
{
153
+
if(Succeeded(Operation3()))
154
+
{
155
+
if(Succeeded(Operation4()))
156
+
returnHResult.Ok;
157
+
else
158
+
returnHResult.Operation4Failed;
159
+
}
160
+
else
161
+
returnHResult.Operation3Failed;
162
+
}
163
+
else
164
+
returnHResult.Operation2Failed;
165
+
}
166
+
else
167
+
returnHResult.Operation1Failed;
168
+
}
169
+
```
170
+
171
+
We got rid of the empty `if` body, making in the meanwhile the code slightly easier to reason about.
172
+
173
+
### Step 3
174
+
If now we invert each `if` statement (the step suggested by Sergio)
175
+
176
+
```csharp
177
+
internal HResult SomeFunction()
178
+
{
179
+
if (!Succeeded(Operation1()))
180
+
returnHResult.Operation1Failed;
181
+
182
+
if (!Succeeded(Operation2()))
183
+
returnHResult.Operation2Failed;
184
+
185
+
if (!Succeeded(Operation3()))
186
+
returnHResult.Operation3Failed;
187
+
188
+
if (!Succeeded(Operation4()))
189
+
returnHResult.Operation4Failed;
190
+
191
+
returnHResult.Ok;
192
+
}
193
+
```
194
+
195
+
we make it apparent that the code performs a chain of executions: if an operation succeeds, the next operation is invoked; otherwise, the chain is interrupted, with an error. The GOF [Chain of Responsibility Pattern][8] comes to mind.
196
+
197
+
### Step 4
198
+
We could move each operation to a separate class, and let our function receive a chain of operations to execute in a single shot. Each class would deal with its specific operation logic (honoring the Single Responsibility Principle).
199
+
200
+
```csharp
201
+
internal HResult SomeFunction()
202
+
{
203
+
var operations =newList<IOperation>
204
+
{
205
+
newOperation1(),
206
+
newOperation2(),
207
+
newOperation3(),
208
+
newOperation4()
209
+
};
210
+
211
+
foreach (var operation in operations)
212
+
{
213
+
if (!_check.Succeeded(operation.DoJob()))
214
+
returnoperation.ErrorCode;
215
+
}
216
+
217
+
returnHResult.Ok;
218
+
}
219
+
```
220
+
221
+
We got rid of the `if`s altogether (but one).
222
+
223
+
Notice how:
224
+
225
+
* The interface `IOperation` has been introduced, which is a preliminary move to decouple the function from the operations, complying the with the Dependency Inversion Principle;
226
+
* The list of operations can easily be injected into the class, using the [Dependency Injection][9].
227
+
* The result validation logic has been moved to a separate class `Check`, injected into the main class (Dependency Inversion and Single Responsibility are satisfied).
We gained the ability to switch the check logic without modifying the main class (Open-Closed Principle).
246
+
247
+
Each operation has been moved to a separate class, like:
248
+
249
+
```csharp
250
+
internal classOperation1: IOperation {
251
+
public string DoJob()
252
+
{
253
+
return"operation1 result";
254
+
}
255
+
256
+
public HResult ErrorCode=>HResult.Operation1Failed;
257
+
}
258
+
```
259
+
260
+
Each operation knows its own error code. The function itself became independent from it.
261
+
262
+
### Step 5
263
+
There is something more to refactor on the code
264
+
265
+
```csharp
266
+
foreach (var operation in operations)
267
+
{
268
+
if (!_check.Succeeded(operation.DoJob()))
269
+
returnoperation.ErrorCode;
270
+
}
271
+
272
+
returnHResult.Ok;
273
+
}
274
+
```
275
+
276
+
* First, it's not clear why the case `returnHResult.Ok;` is handled as a special case: the chain could contain a terminating operation never failing and returning that value. This would allow us to get rid of that last `if`.
277
+
278
+
* Second, our function still has 2 responsibility: to visit the chain, and to check the result.
279
+
280
+
An idea could be to encapsulate the operations into a real chain, so our function could reduce to something like:
281
+
282
+
```
283
+
returnoperations.ChainTogether(_check).Execute();
284
+
```
285
+
286
+
We have 2 options:
287
+
288
+
* Each operation knows the next operation, so starting from operation1 we could execute the whole chain with a single call;
289
+
* Operations are kept unaware of being part of a chain; a separate, encapsulating structure adds to operations the ability to be executed in sequence.
290
+
291
+
I'm going on with the latter, but that's absolutely debatable. I'm introducing a class modelling a ring in a chain, moving the code away from our class:
This class is responsible to execute an operation and to handle the execution to the next ring if it succeeded, or to interrupt the chain returning the right error code.
319
+
320
+
The chain will be terminated by a never-failing element:
In this case, `ChainTogether()` is a function implemented as an extension of `List<IOperation>`, as I don't believe that the chaining logic is responsibility of our class.
351
+
352
+
353
+
## That's not the *right* answer
354
+
355
+
It's absolutely debatable that the responsibilities have been separated to the most appropriate classes. For example:
356
+
357
+
* is chaining operations a task of our function? Or should it directly receive the chained structure?
358
+
* why the use of an enumerable? As Robert Martin wrote in "Refactoring: Improving the Design of Existing Code": enums are code smells and should be refactored to polymorphic classes;
359
+
* how much is too much? Is the resulting design too complex? Does the complexity of the whole application need this level of modularisation?
360
+
361
+
Therefore, I'm sure there are several other ways to refactor the original function.
0 commit comments