Saturday, November 19, 2016

The Cost of Separation of Concerns

One of the things I enjoy doing is looking at other developers' code and dissecting it. You can find a lot of advice on how to refactor code (including this blog). One that caught my eye recently was this one. It shows a small C# example and how to refactor/improve it.

Go ahead and read it. It'll take 5 minutes.

I'll wait.

So I agree that those changes were probably improvements to the code. But when I see code, I see layers and coupling and dependencies. So I thought I'd post a refactoring here that is very different than you'll see in most places.

First, the original code...


 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
 public class Invoice  
 {  
   public long Amount { get; set; }  
   public DateTime InvoiceDate { get; set; }  
   public void Add()  
   {  
     try  
     {  
       // Code for adding invoice  
       // Once Invoice has been added , send mail   
       MailMessage mailMessage = new MailMessage  
 ("MailAddressFrom","MailAddressTo","MailSubject","MailBody");  
       this.SendEmail(mailMessage);  
     }  
     catch (Exception ex)  
     {  
        System.IO.File.WriteAllText(@"c:\Error.txt", ex.ToString());  
     }  
   }  
   public void Delete()  
   {  
     try  
     {  
       // Code for Delete invoice  
     }  
     catch (Exception ex)  
     {  
       System.IO.File.WriteAllText(@"c:\Error.txt", ex.ToString());  
     }  
   }  
   public void SendEmail(MailMessage mailMessage)  
   {  
     try  
     {  
       // Code for getting Email setting and send invoice mail  
     }  
     catch (Exception ex)  
     {  
       System.IO.File.WriteAllText(@"c:\Error.txt", ex.ToString());  
     }  
   }  
 }

And now the original refactored code...


 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
 public class Invoice  
 {  
   public long Amount { get; set; }  
   public DateTime InvoiceDate { get; set; }  
   private FileLogger fileLogger;  
   private MailSender mailSender;  
   public Invoice()  
   {  
     fileLogger = new FileLogger();  
     mailSender = new MailSender();  
   }  
   public void Add()  
   {  
     try  
     {  
       fileLogger.Info("Add method Start");  
       // Code for adding invoice  
       // Once Invoice has been added , send mail   
       mailSender.From = "rakesh.girase@thedigitalgroup.net";  
       mailSender.To = "customers@digitalgroup.com";  
       mailSender.Subject = "TestMail";  
       mailSender.Body = "This is a text mail";  
       mailSender.SendEmail();  
     }  
     catch (Exception ex)  
     {  
       fileLogger.Error("Error while Adding Invoice", ex);  
     }  
   }  
   public void Delete()  
   {  
     try  
     {  
       fileLogger.Info("Add Delete Start");  
       // Code for Delete invoice  
     }  
     catch (Exception ex)  
     {  
       fileLogger.Error("Error while Deleting Invoice", ex);  
     }  
   }  
 }  
 public interface ILogger  
 {  
   void Info(string info);  
   void Debug(string info);  
   void Error(string message, Exception ex);  
 }  
 public class FileLogger : ILogger  
 {  
   public FileLogger()  
   {  
     // Code for initialization i.e. Creating Log file with specified   
     // details  
   }  
   public void Info(string info)  
   {  
     // Code for writing details into text file   
   }  
   public void Debug(string info)  
   {  
     // Code for writing debug information into text file   
   }  
   public void Error(string message, Exception ex)  
   {  
     // Code for writing Error with message and exception detail  
   }  
 }  
 public class MailSender  
 {  
   public string From { get; set; }  
   public string To { get; set; }  
   public string Subject { get; set; }  
   public string Body { get; set; }  
   public void SendEmail()  
   {  
     // Code for sending mail  
   }  
 }

So here is my refactoring. You will see that it is very, very different than the original improved suggested version.

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
105
 interface Invoice  
 {  
   void Add();  
   void Delete();  
 }  
 interface BetterInvoice extends Invoice  
 {  
   void Error(String msg, Exception e);  
 }  
 class InvoiceBasics implements Invoice  
 {  
   public void Add()  
   {  
      // code for adding invoice  
   }  
   public void Delete()  
   {  
      // code for deleting invoice  
   }  
 }  
 class InvoiceLogging implements BetterInvoice  
 {  
   InvoiceLogging(Invoice inner)  
   {  
      this.inner = inner;  
   }  
   public void Add()  
   {  
     fileLogger.Info("Add method Start");  
       inner.Add();  
   }  
   public void Delete()  
   {  
     fileLogger.Info("Add Delete Start");  
      inner.Delete();  
   }  
   public void Error(String msg, Exception e)  
   {  
      fileLogger.Error(msg, e);  
   }  
 }  
 class InvoiceErrorFlow implements Invoice  
 {  
   BetterInvoice inner;  
   InvoiceErrorFlow(BetterInvoice inner)  
   {  
      this.inner = inner;  
   }  
   public void Add()  
   {  
     try  
     {  
        inner.Add();  
     }  
     catch (Exception ex)  
     {  
       inner.Error("Error while Adding Invoice", ex);  
     }  
   }  
   public void Delete()  
   {  
     try  
     {  
        inner.Delete();  
     }  
     catch (Exception ex)  
     {  
       inner.Error("Error while Deleting Invoice", ex);  
     }  
   }  
 }  
 class InvoiceMailer implements Invoice  
 {  
   InvoiceMailer(Invoice inner)  
   {  
      this.inner = inner;  
   }  
   public void Add()  
   {  
      inner.Add();  
     sendMail();  
   }  
   private void sendMail()  
   {  
     mailSender.From = "rakesh.girase@thedigitalgroup.net";  
     mailSender.To = "customers@digitalgroup.com";  
     mailSender.Subject = "TestMail";  
     mailSender.Body = "This is a text mail";  
     mailSender.SendEmail();  
   }  
   public void Delete()  
   {  
      inner.Delete();  
   }  
 }  
 class InvoiceBuilder  
 {  
   public Invoice create()  
   {  
      return new InvoiceMailer(  
        new InvoiceErrFlow(  
           new InvoiceLogging(new(  
             new InvoiceBasics())));  
   }  
 }

I bet you weren't expecting that. I have been a developer for over 30 years and it has taken some time to see software the way I do. So lets evaluate this. Is the code better? It depends.
  • The code above has separation of logging, error flow, and notification and emphasizes composition instead of inheritance.
  • The main business logic (found in InvoiceBasics) is very simple and clean.
  • More functionality can be added without disturbing everything else.
  • It is far more testable than the original.
  • It conforms more to the Single Responsibility Principle than the original.
But there is a cost. There are more classes (that do less). It is probably easier to understand what happens in the original code, where you'll have to think about it with the newly refactored code. It took more time to do than the refactored version, but you gain in testability, flexibility, etc. So you have to take all this into account. You, as the designer, have to decide how much money to spend on this code and how long you expect it to live. It is not always easy to decide.
Posted by at

No comments:

Post a Comment

Subscribe to: Post Comments (Atom)