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.