What not to do: Anti-Patterns and the Solutions
4.84/5 (58 votes)
An introduction to knowing what NOT to do.
Introduction
A while ago, someone posted a link referring to this page about so-called 'Anti-Patterns'[^]. Anti-Patterns are, contrary to Design Patterns[^], recurring programming practices that create problems instead of solving them. Going through that page, I found many patterns I used when I just started out programming (the shame!). Luckily, I have learned a lot since, and I think I do not use any of the Anti-Patterns anymore. However, I do see many people on the web who still use these counterproductive techniques, either knowingly or unknowingly. Many articles on CP and other web sources discuss various Design Patterns. Anti-Patterns are usually not discussed though.
So Design Patterns solve problems, but why would you solve problems if you are not really aware of any? Well, you might actually already have a problem. I believe that knowing the problem is already half of the solution. But to acknowledge a problem, you have to know that there is a problem in the first place. So knowing what not to do might be as important as knowing what to do. This is exactly why I have written this article. I want to point out to (beginning) programmers what not to do and how to do things instead. I have taken some Anti-Patterns from this page[^] which I will discuss and which I have used in a sample project (and also corrected).
"Good programmers use their brains, but good guidelines save us having to think out every case." -- (Francis Glassborow)
Background
Knowing why not to use these Anti-Patterns, or why they are Anti-Patterns in the first place, requires some knowledge on Object Oriented Programming[^]. While the basics of Object Oriented Programming, the so called SOLID Principles[^], are not very hard to understand (assuming some programming knowledge), they are hard to put to practice (and you might never do it completely right). For this reason, I will not go into the details of these SOLID Principles. For now, it is important to know that using these principles allows for more organised code and (Class structure, which makes your application more stable and maintainable. As a bonus, it becomes easier to implement Design Patterns into your software which makes it easier to solve common programming problems. So, as you might expect, Anti-Patterns are the opposite of proper SOLID Principles and Design Patterns, and make your code harder to read, less stable, and could make a hell out of maintaining the code.
"There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies. And the other way is to make it so complicated that there are no obvious deficiencies." -- (C.A.R. Hoare)
Using the code
For this article, I have created two sample projects. One in Visual Basic and one in C#. Why? Because I have been using VB for quite some months now and to me still feels more familiar than C#. But because a lot of people here might be more familiar with C#, I have chosen to translate the project into C# as well. That was a good syntactic workout for me as well. I have finally decided to put the C# code in the article and add the VB project as a bonus.
So looking at the project, what do we see? We see a Windows Form with a few tabs on the left, many buttons and a textbox in the right. While the many buttons do the same thing, they handle this differently. One button might do action A using an Anti-Pattern while another button does the exact same thing, but having (partly) solved the Anti-Pattern. Debugging the code is key in understanding what is happening here! The code is heavily commented so there is no excuse to not go into that debugger :)
The TextBox on the right functions as a logging mechanism and shows the user when something goes wrong, among other things.
I have to mention that I have tried to keep the project simple and easy to understand for everyone. For this reason, I have not used proper Object Oriented Design, which would have been a necessity for what I am doing here. Instead, try to look at this project as an absolute beginner's first step towards Object Oriented Programming.
In the project, I have faked an email client. Instead of sending emails, I do some string concatenation and prompt the user with MessageBoxes representing this email (you should never actually use MessageBoxes in your Classes other then a Windows.Forms.Form Class). However, things might go wrong (I programmed it that way). With the use of Anti-Patterns, this could be problematic (as the TextBox on the right shows). When we have solved the Anti-Patterns, things might still go wrong, but in this case, this poses less of a problem (remember that every application will go wrong at one point or another).
"Controlling complexity is the essence of computer programming." -- (Brian Kernigan)
Magic Pushbutton and Copy/Paste Programming Anti-Patterns
The first of the Anti-Patterns I want to discuss is the Magic Pushbutton Anti-Pattern[^]. Start the sample project and browse to the "Abstractions" tab. Why is this called the "Abstractions" tab? Find out in a couple of minutes. First, push the button "Send Test Email" in the "Magic Pushbutton" groupbox. What do you get? An email to fubar@customer.com with the message "This is a test email!" and the subject "Test", all neatly formatted in a messagebox. Also notice that the textbox (which represents the log) has a first entry "-- Email was sent." Wow, that is really amazing! Let us see what code is underneath that button.
private void btnSendTestEmail_Click(System.Object sender, System.EventArgs e)
{
try
{
string mail = "To: fubar@customer.com{0}{0}This is a test email!";
// Show a MessageBox representing the email.
MessageBox.Show(string.Format(mail, Environment.NewLine),
"Test", MessageBoxButtons.OK, MessageBoxIcon.Information);
// Log to the UI. Not best practice, but it will do for the example.
this.txtLog.Text += "-- Email was sent." + Environment.NewLine;
}
catch (Exception ex)
{
// Show the user that something went wrong.
MessageBox.Show(ex.Message, this.Text, MessageBoxButtons.OK,
MessageBoxIcon.Error);
// Then log the Message. Imagine someone forgetting
// the -- or NewLine. Your log would be a mess!
this.txtLog.Text += "-- " + ex.Message + Environment.NewLine;
}
}
So first we see that the receiver of the email is fubar@customer.com, the subject is "Test" (written in the MessageBox's parameters) and the body is "This is a test email!" (not really, just imagine it to be). Next, the txtLog gets a new Entry "-- Email was sent.". This looks pretty good, right? The code does what it should do. Not really.
The code is implemented directly in the Button's Event Handler, making the code useless if the Button was not there. This is called the Magic Pushbutton Pattern. You press a Button and some magic happens. This has several disadvantages and no advantages at all! The first disadvantage is that the code can grow to unmanageable proportions. Your customer might call that they want to send an attachment with the email and that they possibly want to send a copy to their own email address and that they want to have a cup of coffee when the email is sent. Your Event Handler will grow to some tens of lines of code... Second, what if you wanted to do the exact same thing with another Button or Control or on another Form or even in another application? What if we wanted to change the receiver, subject, or body of the email? It is simply not possible. This Button Event Handler cannot be called with different values for any of those. The code is ONLY usable for this specific Button.
So what if we still wanted to send another email? We are going to use another Anti-Pattern for that, called the Copy/Paste Programming Anti-Pattern[^]. But first, click the second Button. It is located in the GroupBox "Copy / Paste" and has the caption "Send Test Email with Attachment". This Button shows a MessageBox (email) which looks largely the same as the first one. With a slight difference. The subject and body have changed and an attachment has been added to the email. Let us look at the code again:
private void btnSendTestEmailWithAttachment_Click(System.Object sender, System.EventArgs e)
{
try
{
string mail = "To: fubar@customer.com{0}{0}Attachment: ";
// Add the attachment here.
string attachment = @"C:\\Reports\TestAttachment.txt";
mail += attachment;
this.txtLog.Text += string.Format("--The attachment {0} " +
"has been added to the email.{1}",
attachment, Environment.NewLine);
// Add the body to the mail.
mail += "{0}{0}This is a test email with Attachment!";
// Show a MessageBox representing the email.
MessageBox.Show(string.Format(mail, Environment.NewLine),
"Test with Attachment", MessageBoxButtons.OK,
MessageBoxIcon.Information);
// Log to the UI. Not best practice, but it will do for the example.
this.txtLog.Text += "--Email was sent." + Environment.NewLine;
}
catch (Exception ex)
{
// Show the user that something went wrong.
MessageBox.Show(ex.Message, this.Text, MessageBoxButtons.OK,
MessageBoxIcon.Error);
// Then log the Message. Imagine someone forgetting
// the -- or NewLine. Your log would be a mess!
this.txtLog.Text += "--" + ex.Message + Environment.NewLine;
}
}
Notice two things. Half of the code presented in the second Buttons Event Handler is the same as that in the first Buttons Event Handler. Why? Because I have copy/pasted it! The part that is not the same is that where I include the attachment (which requires some extra string concatenation). Doing this might seem fine, because the email is still correctly send (or in our case correctly formatted in the MessageBox). But notice how the logging is inconsistent with the first Buttons log. To be honest I have included the "-- " in front of each log later then when I first created these two Button Event Handlers. This meant that I had to correct BOTH Event Handlers to format the log message correctly, but I have forgotten one Handler. This is really not very stupid of me, it is really very difficult to find every log action like this, so this approach is just very prone to errors (pray that it sticks to a space in a log message, I have made complete Forms unusable by copy/pasting...). So this Button has everything that is wrong with the first Button, but it is also not pasted entirely correct. How to solve these problems (and now you will know why that first tab is called "Abstraction")? We need to abstract these actions away from the Button Event Handlers.
Let us first take a look at how I have abstracted the logging away from the Handlers:
private void LogMessage(string message)
{
// Writes a message to the log window.
this.txtLog.Text += "-- " + message + Environment.NewLine;
}
I have created a Method that returns void (Sub in VB) which takes a string as parameter. It adds this string (and "-- ") to the log. See how this could never go wrong? The only thing that could go wrong is that a programmer passes an unintelligible message as parameter. However, this is quite unlikely.
Now how about those email functions? How could we make those more abstract so that they can be used by other Buttons in our Form too?
private void SendSimpleMail(string subject, string message, string email)
{
string mail = "To: {1}{0}{0}{2}";
// Show a MessageBox representing the email.
MessageBox.Show(string.Format(mail, Environment.NewLine, email, message),
subject, MessageBoxButtons.OK, MessageBoxIcon.Information);
// Log to the UI. Not best practice, but it will do for the example.
LogMessage("Email was sent.");
}
private void SendMailWithAttachment(string subject, string message,
string email, string attachment)
{
string mail = "To: {1}{0}{0}Attachment: ";
// Add the attachment here.
mail += attachment;
LogMessage(string.Format("The attachment {0} has " +
"been added to the email.", attachment));
// Add the body to the mail.
mail += "{0}{0}{2}";
// Show a MessageBox representing the email.
MessageBox.Show(string.Format(mail, Environment.NewLine, email, message),
subject, MessageBoxButtons.OK, MessageBoxIcon.Information);
// Log to the UI. Not best practice, but it will do for the example.
LogMessage("Email was sent.");
}
I have created two Methods that return void (Subs in VB) which do the exact same thing as the first two Buttons Event Handlers. However, these Methods expect a subject, message and email address to be passed as parameters. This means I could use these Methods from every Button (or any other Control) in my Form and they would always do the same thing. Also note that the newly created LogMessage MethodButtons in the "Abstractions" tab. They do the exact same as the first two Buttons, but the logging is now correct and the Buttons Event Handlers look very different.
private void btnSendTestEmail2_Click(System.Object sender, System.EventArgs e)
{
try
{
SendSimpleMail("Test", "This is a test email!",
"fubar@customer.com");
}
catch (Exception ex)
{
// Show the user that something went wrong.
MessageBox.Show(ex.Message, this.Text, MessageBoxButtons.OK,
MessageBoxIcon.Error);
// Then log the Message.
LogMessage(ex.Message);
}
}
private void btnSendTestEmailWithAttachment2_Click(System.Object sender,
System.EventArgs e)
{
try
{
SendMailWithAttachment("Test", "This is a test email!",
"fubar@customer.com", @"C:\\Reports\TestAttachment.txt");
}
catch (Exception ex)
{
// Show the user that something went wrong.
MessageBox.Show(ex.Message, this.Text, MessageBoxButtons.OK,
MessageBoxIcon.Error);
// Then log the Message.
LogMessage(ex.Message);
}
}
In fact, most of the code in the Handlers is now showing a possible Exception to the user and logging this. The sending of the email, with or without attachment, is now handled away from the Buttons Event Handler. The code has now become more reusable. This is a first step in solving the Magic Pushbutton and Copy/Paste Programming Anti-Patterns! Abstracting Methods like this is a good practice. However, in our current code we still have the Methods in our Form. So if we wanted to use these Methods in another Form or application we are still in a bind. On top of that, the one Method is still largely a copy of our other Method. Our current solution is still not sufficient enough, we have to move away the Methods in a seperate Class and cut the creating, sending and adding of attachments to an email into different pieces of code.
Object Orgy Anti-Pattern
In this part I have moved away the emailMethods in a seperate Class. I will not go over this in detail, but take a look at how I have done this. Of course I have done this in a bad way, using the Object Orgy Anti-Pattern[^]. An Object Orgy means that an Object is insufficiently encapsulated[^], meaning that the Object can be accessed how or where you do not want it to be accessed. Encapsulation[^] is one out of three primary characteristics of Object Oriented Programming (together with Inheritance and Polymorphism) and I suggest you learn it well. You will see why encapsulation is so important in the next example.But first, let us take a look at how I have implemented the former two
Methods into a new Class called "BadEmailClass" (comments reduced for readability here):
public static List<string> Attachments { get; set; }
public static void AddAttachment(string filePath, LogMessage logger)
{
// Make sure the List<string> is not null.
if (Attachments == null)
{
Attachments = new List<string>();
}
// Add the file.
Attachments.Add(filePath);
logger(string.Format("The attachment {0} has been " +
"added to the email.", filePath));
}
public static void SendMail(string Subject, string message,
string email, LogMessage logger)
{
string mail = "To: " + email + "{0}";
if (Attachments != null && Attachments.Count > 0)
{
mail += "{0}Attachments:{0}";
foreach (string attachment in Attachments)
{
mail += attachment + "{0}";
}
}
mail += "{0}" + message;
try
{
// An Exception MAY occur. This Class Catches it,
// Handles it, and you will never notice!
Random rand = new Random();
if (rand.Next(1, 5) == 1)
{
// Throw an Exception with some meaningless Message.
throw new Exception("An Error has occurred");
}
// Calling a MessageBox from a Class... Not done in real worls apps!
MessageBox.Show(string.Format(mail, Environment.NewLine), Subject,
MessageBoxButtons.OK, MessageBoxIcon.Information);
logger("Email was sent.");
}
catch (Exception ex)
{
logger(ex.Message);
}
}
First notice that there is no "SendMailWithAttachment" anymore. Instead this Class contains a List<string>List(Of String) in VB) to which attachments can be added using the "AddAttachment" Method. When all the attachments are added you can call "SendMail" and the mail will automatically be send, including all added attachments. Second, also notice that the "SendMail" Method may produce an Exception. Last, I pass a Delegate to each Method which takes care of logging on my Form. Do not worry about the Delegate, it is not important for the example and the point I am trying to make. If you want to know more about Delegates refer to this page[^] for information. It should also be said (again) that using MessageBoxes in Classes other than System.Windows.Forms.Form is simply not done. By doing so your Class can only be used in WinForm projects and programmers using your Class have to force the user of the application to click away yet another MessageBox. I have declared both Methods and the List<string> as static (Shared in VB) so anyone who wants to use this Class can simply use it without creating an instance of the Class (I thought this was really handy when I first found out about it!). Before you press any Button I want you to look at the code that uses this Class (again, comments removed).
private void btnSendInvoiceBad_Click(System.Object sender, System.EventArgs e)
{
try
{
string mailBody = "Dear Customer,{0}Please, " +
"PAY UP!!!{0}Regards,{0}Your Supplier";
BadEmailClass.AddAttachment(@"C:\\Reports\Invoice.rpt", LogMessage);
BadEmailClass.SendMail("Invoice",
string.Format(mailBody, Environment.NewLine),
"fubar@customer.com", LogMessage);
}
catch (Exception ex)
{
// Show the user that something went wrong.
MessageBox.Show(ex.Message, this.Text,
MessageBoxButtons.OK, MessageBoxIcon.Error);
LogMessage(ex.Message);
}
}
private void btnSendReminderBad_Click(System.Object sender, System.EventArgs e)
{
try
{
// The mofo better pay up!
string mailBody = "Dear Customer,{0}You still have not " +
"shown us the money!{0}Regards,{0}Your Supplier";
BadEmailClass.Attachments.Add(@"C:\\Reports\Reminder.rpt");
BadEmailClass.SendMail("Reminder",
string.Format(mailBody, Environment.NewLine),
"fubar@customer.com", LogMessage);
}
catch (Exception ex)
{
// Show the user that something went wrong.
MessageBox.Show(ex.Message, this.Text, MessageBoxButtons.OK, MessageBoxIcon.Error);
LogMessage(ex.Message);
}
}
So assume now that you press the first Button, an attachment gets added to an email and the email is send. Then assume you press the second Button. Again, an attachment is added and the email is send. Any Exceptions will be caught, shown to the user and logged. So little code and all seems perfect! Now go to the "Object Orgy" tab and press the Buttons in the "Bad Email Class" GroupBox a couple of times...
You will notice a few things: the log sometimes displays an error message, but you are not notified (you know an error has occurred because you do not see the MessageBox). The number of attachments keeps growing when you press the Buttons more often. An attachment that was added in the one Button shows up in the email of the other Button and vice versa. And when sending a reminder to the customer the adding of an attachment does not get logged. In short, it is a mess!
So what goes wrong here? Averything in this Class is declared static (Shared in VB) which gives other Classes (like our Form) unrestricted access to our BadEmailClass's internal values (in this example the List). The ListDisposed after an email has been sent. Actually, because it is static it will never be Disposed by the Garbage Collector because any other Class might access it at any given time and use its items. So we could have the SendMail Method empty the List? No, because what if we wanted to send a copy of the email to another email addess? The attachments will be gone. So we should force the user of our Class to empty the list before using the Class? No, because this leads to another Anti-Pattern, Sequential Coupling[^], which means that a programmer has to call any Methods in the Class Class will not work or show unexpected behaviour. What we have here is an Object Orgy. The Methods and Fields of this Class have been improperly encapsulated. I have solved this problem in the GoodEmailClass. Watch how the Methods and ListClass:
private List<string> _attachments;
public GoodEmailClass()
{
_attachments = new List<string>();
}
public void AddAttachment(string filePath, LogMessage logger)
{ //... }
public void SendMail(string Subject, string message, string email, LogMessage logger)
{ //... }
I have removed the static (Shared in VB) keyword and made the ListMethods Public. Doing this will solve almost all our problems. Let us see how it is now used in our Form:
private void btnSendInvoiceGood_Click(System.Object sender, System.EventArgs e)
{
try
{
string message = string.Format("Dear Customer,{0}Please, " +
"PAY UP!!!{0}Regards,{0}Your Supplier", Environment.NewLine);
SendMail("Invoice", message, "fubar@customer.com",
@"C:\\Reports\Invoice.rpt");
}
catch (Exception ex)
{
MessageBox.Show(ex.Message, this.Text,
MessageBoxButtons.OK, MessageBoxIcon.Error);
LogMessage(ex.Message);
}
}
private void btnSendReminderGood_Click(System.Object sender, System.EventArgs e)
{
try
{
string message = "Dear Customer,{0}You still have not " +
"shown us the money!{0}Regards,{0}Your Supplier";
SendMail("Reminder", message, "fubar@customer.com",
@"C:\\Reports\Reminder.rpt");
}
catch (Exception ex)
{
MessageBox.Show(ex.Message, this.Text, MessageBoxButtons.OK, MessageBoxIcon.Error);
LogMessage(ex.Message);
}
}
private void btnSendFunnyJokes_Click(System.Object sender, System.EventArgs e)
{
try
{
SendMail("Funny!", "Some funny stuff!!!",
"friend@fubar.com", "funnypic1.jpg",
"funnypic2.jpg", "funnyvid.mov");
}
catch (Exception ex)
{
MessageBox.Show(ex.Message, this.Text,
MessageBoxButtons.OK, MessageBoxIcon.Error);
LogMessage(ex.Message);
}
}
// Overloaded Method, in case you do not want to send any attachments.
private void SendMail(string subject, string message, string emailAddress)
{
SendMail(subject, message, emailAddress, null);
}
private void SendMail(string subject, string message,
string emailAddress, params string[] attachments)
{
// We have to instantiate a new Object. This will
// start from scratch (no unwanted surprises).
GoodEmailClass email = new GoodEmailClass();
if (attachments != null)
{
foreach (string attachment in attachments)
{
email.AddAttachment(attachment, LogMessage);
}
}
email.SendMail(subject, message, emailAddress, LogMessage);
}
As you can see the new implementation forces us to create a new instance of our Class, which also makes sure a new List is being created. It is now also possible to add as many attachments as you want, from wherever you want, as long as you have a reference to your GoodEmailClass. This is easier to debug, since it is now clear where your GoodEmailClass instance is used. You can even find all references by right-clicking on the email variable and clicking "find all references" in Visual Studio. Now click the Buttons in the "Good Email Class" GroupBox and see how everything gets logged, how you are notified about any Exceptions and how every email you send has only the attachments you expect it to have!
Class' encapsulation what is up with those Exceptions? Why do we see when an Exception occurs in our GoodEmailClass, but not in our BadEmailClass? That is because our BadEmailClass had another Anti-Pattern implemented, the Error Hiding Anti-Pattern[^]. This means that our BadEmailClass produced an Exception, but never passed the Exception to the user of the Class (our Form). Instead it caught and handled it (which is fine, but make sure the 'top layer' always gets the Exception too). On top of that, even if it did Throw the Exception up to out Form, we would get an Exception message that would be useless "An Error has occurred.". The GoodEmailClass, on the contrary, does not catch the Exception at all and it gives us a detailed error description. For more info on correct Exception Handling read this article[^].
The God Object
Last, but not least, in this article about Anti-Patterns is the God Object Anti-Pattern[^]. With the GoodEmailClass our application was actually pretty neat. We sold it to a satisfied customer and this customer has now asked us for some additional functionality! If that is not good news then what is!? This customer wants our application to be able to send SMS'es to telephones and to be able to set email and SMS privileges per employee. So we start programming and before you know it we have added the following to our GoodEmailClass (I have copied the GoodEmailClass to the GodClass and added the following to it):
public static void SendSms(string phoneNumber,
string message, LogMessage logger)
{
// Check if the telephone number is of a correct format.
if (!Regex.IsMatch(phoneNumber, @"^\d+$") || phoneNumber.Length != 10)
{
throw new System.Exception(phoneNumber +
" is not recognized as a valid telephone number.");
}
MessageBox.Show(message, phoneNumber,
MessageBoxButtons.OK, MessageBoxIcon.Information);
logger("SMS was sent.");
}
// This is a user setting.
// Probably stored somewhere in a DB or config file.
protected static bool _canSendEmail = true;
public static bool CanSendEmail
{
get { return _canSendEmail; }
}
// This is a user setting. Probably stored somewhere in a DB or config file.
protected static bool _canSendSms = true;
public static bool CanSendSms
{
get { return _canSendSms; }
}
The implementation in our Form for sending emails has remained largely untouched (I re-implemented it using the GodClass and now check if CanSendEmail is true). The implementation for the SMS function looks like this:
private void btnSendGodSms_Click(System.Object sender, System.EventArgs e)
{
try
{
SendGodSms("0612345678", "This is a text message!");
}
catch (Exception ex)
{
MessageBox.Show(ex.Message, this.Text,
MessageBoxButtons.OK, MessageBoxIcon.Error);
LogMessage(ex.Message);
}
}
private void SendGodSms(string phoneNumber, string message)
{
// First check if the user is allowed to send an SMS.
if (GodClass.CanSendSms)
{
// The SMS is send and all is fine.
GodClass.SendSms(phoneNumber, message, LogMessage);
}
else
{
// If the user is not allowed we prompt him.
// Usually the user would not be able
// to press any 'Send SMS' button at all.
MessageBox.Show("You are not allowed to send an sms.",
this.Text, MessageBoxButtons.OK,
MessageBoxIcon.Hand);
LogMessage("SMS was denied.");
}
}
We first have to check if the user is allowed to send an SMS. In most cases I think it is better to hide or disable the SMS Button if the current user is not allowed to send SMS, but for now let us do it this way. So that is that. We have just created our first God Object! A God Object is an all-knowing Object, or an Object that knows or does to much. In this example we have a Class that sends emails and SMSes and also stores user settings. Tempting though it may be, this is bad practice. Yes, emailing and SMSing are both forms of communication so they should (could) be kept together and yes, the user settings have everything to do with emailing and SMSing. But try to maintain this code. Whenever new functionality is needed you will be tempted to also put it int this GodClass (it has lots of functionality already anyway). Also, chances are that when you alter the SMS functionality your email functionality will break. Even when you think it will never happen it will somehow happen. Changing a Class may cause the whole Class to behave differently if you are not careful. Therefore it is best to keep a Class responsible for one thing only (Single Responsibility Principle, the S in SOLID Principles!). Also, a new programmer on the team would never expect to find SMS functionality in a Class called "Email". Calling your Class "Communications" will not clear things up. After a couple of months, or even years, large parts of your Object will not be used anymore (legacy code) and the unused parts cannot be kept apart from the used parts. Maintaining God Objects can be tiresome and confusing because seperate functionalities become intertwined.
Imagine a potential customer calling:
c: "Hello, this is a potential customer speaking."
you: "Hello, we are a potential supplier!"
c: "Can you make me an application that can send emails?"
you: "Sure, do you want SMS functionality with that?"
c: "No..."
you: "Well, you are getting it anyway!"
*Customer hangs up the phone*
You do not want that.
So what is the solution to this? Just lots and lots of Classes (probably including BaseClasses and Interfaces, but that is Object Oriented Design, I would not talk about that). Keep related Classes together, both physical on your hard disk by using folders, and in your code by using Namespaces (which I did not use in my sample projects). As you can see in the sample project I have divided the GodClass into the SmsClass and UserSettings Class. We already had a GoodEmailClass, this will still be used. The implementation of our SMS functionality in our Form now looks like this:
private void btnSendMultipleClassesSms_Click(System.Object sender, System.EventArgs e)
{
try
{
SendSms("0612345678", "This is a final text message!");
}
catch (Exception ex)
{
MessageBox.Show(ex.Message, this.Text,
MessageBoxButtons.OK, MessageBoxIcon.Error);
LogMessage(ex.Message);
}
}
private void SendSms(string phoneNumber, string message)
{
// First check if the user is allowed to send an SMS.
if (UserSettings.CanSendSms)
{
SmsClass.SendSms(phoneNumber, message, LogMessage);
}
else
{
MessageBox.Show("You are not allowed to send an sms.",
this.Text, MessageBoxButtons.OK, MessageBoxIcon.Hand);
LogMessage("SMS was denied.");
}
}
An example where a wrong telephone number is supplied is also available in the project. Somewhat confusing may be the UserSettings class. It is. The Properties are static (Shared in VB) while the members it holds are Protected. Simply creating a new instance of the UserSettings Class everytime you want to check if a user has permissions to perform an action is not going to work, because this will reset all settings. You could set the permissions again everytime you instantiate the Object, but in this case I want the user to restart the application if his permissions changed. So I only want to set the permissions when the application starts up. For such a problem (where only one instance of a Class must exist at all times) we might use the Singleton Design Pattern[^] (and notice how we can use this Pattern now that we do not have a God Object anymore!). But I promised you that this would be an article about Anti-Patterns, not Design Patterns. So I will leave you figure that one out on your own. So we see that the code is now better readable and that Classes have logically divided functionality. Changing SMS functionality will never ever hurt our emailing functionality because we are working in physically different files and Classes (without dependencies). Now go press some Buttons on "Gob Object tab" :)
Conclusion
Though many of the Anti-Patterns discussed in this article might seem like simple mistakes that can be easily solved, they are common mistakes that can hurt any project (to failure even). The truth is that these 'simple, little mistakes' add up to a point where they cannot be 'easily fixed' anymore. The mistakes can be subtle and anyone can make them, such as making a Field Public instead of Private or forgetting to Throw an Exception. People that have just started to learn the language may not see or understand the subleties of these errors and programmers with years of experience may simply have an off moment and make them by mistake. By pointing them out and by showing the dangers of these mistakes, Patterns even, I hope people will be more aware when next time they declare a Field Public because someone might want to change a value or catch an Exception without rethrowing it because who would want to know.
Points of Interest
The sample project I have provided is still far from what it should be. Proper Object Oriented Design is difficult and I did not want to trouble the reader with this. However, the first steps towards Object Oriented thinking have been established. I can recommend anyone who is not yet completely familiar with encapsulation principles to practice with it before venturing into Object Oriented Design. Also learn on how to use Inheritance and composition[^], which I have not even discussed in this article. The use and understanding of Interfaces[^] and Polymorphism[^] is of utmost importance. If all of the mentioned terms sound like gibberish to you learn about those first. If you want to learn more on Object Oriented Design and Design Patterns I recommend the following articles next to the literature already referenced in the article:
- How I explained OOD to my wife[^]
- How I explained Design Patterns to my wife: Part 1[^]
- Interfaces in C# (For Beginners)[^]
- Abstract Class versus Interface[^]
- Decorater Pattern (A Layman to Laymen)[^] (perhaps to abstract the logging away from emailing and SMSing in our example project)
- Illustrated GOF Design Patterns in C# Part I: Creational[^] (also look for the other parts in the series)