Tim Barcz : Principles

Poor Design Is Not a Bug

Tim Barcz — Fri, 26 Nov 2010 03:37:18 GMT

Ayende recently posted a an article titled “Where is the bug”. While reading the comments, I was disappointed by some of the answers to Ayende’s request to find the bug. Here are two of the comments that stood out to me:

“You could argue the biggest bug is the use of a switch statement as it violates OCP.“
“The use of a switch statement is a bug in itself.“

What bothers me so greatly is people’s inability to separate design discussion with bugs. A bug is defined on Wikipedia as:

A software bug is the common term used to describe an error, flaw, mistake, failure, or fault in a computer program or system that produces an incorrect or unexpected result, or causes it to behave in unintended ways

The two comments above don’t attempt to solve the bug and instead take an “ivory tower approach” to the problem (more befuddling is that removing the switch in favor of something else would leave the bug in place). This bugs me because we (as an industry) continue to care very little about the output and meeting of customer/business needs and more about code structure and semantics than actually working code.

Let me state for the record, it is perfectly acceptable to talk about code structure and design but please let’s not confuse a bug – an actual error or flaw in code – with poor design.

(I think it’d be humorous for Ayende to post some perfectly fine code and ask “Where’s the bug” and watch people trip over themselves trying to find something wrong)

Ralph Waldo Emerson Would’ve Been a Great Developer

Tim Barcz — Mon, 14 Dec 2009 18:06:36 GMT

A very short quote I came across to start your week off and get you thinking about technology with the correct frame of reference:

“As to the methods there may be a million and then some, but principles are few. The man who grasps principles can successfully select his own methods. The man who tries methods, ignoring principles, is sure to have trouble.”

- Ralph Waldo Emerson (1803-1882)

Mr. Emerson would surely have made a great developer. We need to heed his advice.

As an exercise: think of how many different “methods” you can think of and then think of how many “principles” you can come up with.

Principles:

Single Responsibility Principle
Don’t Repeat Yourself
Dependency Inversion Principle
Liskov Substitution Principle
Open Closed Principle
Interface Segregation Principle

Methods:

ASP.NET
C# 2.0
C# 3.0
LINQ
LINQ to SQL
Entity Framework
NHibernate
SOA
StructureMap
Windsor/MicroKernel
Ninject
ASP.NET MVC
Ruby
IronRuby
W*F
Silverlight
And on and on and on…

“As to the methods there may be a million, but principles are few.”

Real Life - Code Reuse

Tim Barcz — Mon, 09 Mar 2009 18:37:16 GMT

A few months ago I blogged about how we're using the single responsibility principle in our application at work. I'm going to make an effort here to write more "Real Life" content. Often when a developer read principles or ideas in books, there is disconnect between what he/she is reading and how an implementation would look in the real world. The goal of these "Real Life" scenarios is to show areas where I feel I've reaped some benefit from following a specific pattern or practice. This does not mean that my implementation is the only way or always "book correct". The goal is to act as a bridge (quite possibly a old rickety bridge) between the book knowledge and the real world.

Moving along...

One of the purported benefits of OOP has always been the abundant reuse of code. However, in my experience many applications fail to realize any amount of reuse beyond copying and pasting between applications. To really take advantage of code reuse you have to have small, concise pieces which you can rearrange in order to make a new behavior. As a side note, and I want this be clear, many of the principles of good software development, ie. SOLID, go hand-in-hand. You can't do one without the other and the benefits of one cannot be fully realized without the implementation of the others. You'll notice that a few sentences ago I said "small, concise pieces". A light bulb may have gone off in your head thinking "Single Responsibility Principle", and if it did, bonus points for you.

The key to code reuse is to build your components in a very focused way. By doing so, you can add functionality by combining different pieces of you application.

Consider the following code used to authenticate a user into a site:

   1: protected void btnSubmit_Click(object sender, EventArgs e)

   2: {

   3:     string Username = Server.HtmlEncode(txtUsername.Text);

   4:     string Password = Server.HtmlEncode(txtPassword.Text);

   5:     int nUserID;

6:

   7:     if (Username != "" && Password != "")

   8:     {

   9:         using (SqlConnection cnn = new SqlConnection(connectionString))

  10:         {

  11:             using (SqlCommand cmd = cnn.CreateCommand())

  12:             {

  13:                 cmd.CommandText = "usp_Login";

  14:                 cmd.Parameters.AddWithValue("@Username", Username);

  15:                 cmd.Parameters.AddWithValue("@password", password);

16:

  17:                 using(SqlDataReader reader = cmd.ExecuteReader())

  18:                 {

  19:                     if (reader.Read())

  20:                     {

  21:                         nUserId = Convert.ToInt32(reader[0]);

  22:                     }

  23:                 }

  24:             }

  25:         }

26:

  27:         if (nUserID > 0)

  28:         {

  29:             // if the user has checked the box

  30:             // store his information so he doesn't have to log in again

  31:             if (cbRememberMe.Checked)

  32:                 FormsAuthentication.SetAuthCookie(nUserID.ToString(), true);

  33:             else

  34:                 FormsAuthentication.SetAuthCookie(nUserID.ToString(), false);

35:

  36:             BusinessLogicLayer.User objUser = new User(nUserID);

37:

  38:             // store session variables for later reference;

  39:             Session["UserID"] = nUserID;

  40:             Session["Username"] = objUser.Username;

  41:             Session["Password"] = objUser.Password;

  42:             Session["AccessLevel"] = objUser.AccessLevel;

  43:             Session["AccessLevelName"] = Enum.GetName(typeof(Utilities.AccessLevel),objUser.AccessLevel);

44:

  45:             Response.Redirect("~/admin/admin.aspx");

  46:         }

  47:         else

  48:         {

  49:             View = Views.Failed;

  50:         }

  51:     }

  52: }

While the code above is functional, it does way too much. In this button click event there are:

Call to database explicitly
logic to set an authorization cookie
Some session variables being set

Imagine for a moment that you now want to implement a new piece of functionality, user impersonation. In order to accomplish this with the path of least resistance, which developers commonly take, would be to copy/paste all of the login logic to another UI event somewhere else in the site. The issue copy/paste development is that any future change to the login logic has to be copied again or else the impersonation feature isn't really impersonating a users experience. The insidious nature of copy/paste development is that while often it works, it creates a brittle application.

In a recent application we were wanting to add user impersonation. Below is an example of an implementation of user impersonation. Adding the feature to the application was gleefully simple. The ease in adding impersonation was due entirely to the proper granularity in our application components. Seriously, this is all the code it took:

   1: public ActionResult ImpersonateUser(int id)

   2: {

   3:     var authService = Container.Resolve<AuthService>();

   4:     var userRepository = Container.Resolve<UserRepository>();

5:

   6:     var userToImpersonate = userRepository.GetById(id);

   7:     if (userToImpersonate != null)

   8:     {

   9:         authService.SignIn(userToImpersonate);

  10:     }

11:

  12:     return RedirectToAction("Index", "Home");

  13: }

I did not know we were going to want user impersonation for this application when we started. However because we developed the site with certain principles in mind, we were able to reuse pieces of code in ways not originally intended for added behavior. As you can see from the code above, there are no "clever hacks" or tricks to get impersonation to work. The beauty of the code above is that if we change the implementation of "SignIn(user)" all appropriate areas will follow the same rules since all areas use the same code.

As I'm closing I want to challenge those of you who have read this far to stop yourself the next time you find yourself copying code from one location to another. Is there an abstraction or component you're overlooking? Think about the long term and ask if you're really making the wisest decision for the long term health of your application. It will be through this introspection and honesty that you'll find areas where you can write smaller component which will allow you to realize the oft spoken benefit in OOP of code reuse.

Real Life Single Responsibility Principle

Tim Barcz — Mon, 05 Jan 2009 20:23:47 GMT

S.O.L.I.D. principles seem to be a hot topic. The guys over at LosTechies.com explored it last year in March as their topic of the month and did a great job of it. I know when I read blogs I enjoy when different authors post on similar topics. I find that often the different examples and explanations paint a clearer picture than can be had with only one example. I'd like to share an area where we use the Single Responsibility Principle (SRP) in our application

Note: The "aha" moment won't be some thrilling display of how we averted some disastrous bug or how we eliminated some thousands of lines of code. It will be much more subtle but that's ok. A good design should strive for an element of simplicity. Great design is asking to hear the answer to a riddle and then thinking, "duh! of course" When you hear the answer you realize how "obvious" it was.

Background

In our application we queue all emails to be handled by a different process since sending email is a blocking call which slows your response times (Note to BCL junkies: yes I know there is a SendAsync method). There are a few instances in our application where we want email to be sent right away and not queued. An example of this would be a confirmation email for a new account or a password reset request.

The Fix

We already had a QueuedEmailService class so adhering to SRP, I built a RealTimeEmailService class whose job, and only job, it is to send the email right away. Thinking of responsibility driven design, classes:

Know things
Do things
Make decisions

With that in mind I needed another class to make the decision to when to send the email through the RealTimeEmailService or when to use the QueuedEmailService:

(At this point some of you might be reading thinking how easy of a problem this would be to solve and you're quite right. What I want to point out is not the problem, but how one solves the problem. Nearly anyone could've gone into the QueuedEmailService and added an if statement and sent emails real-time if that is what is needed, thus ignoring SRP. By lumping all of the work together you aren't building object oriented software...sorry. And while you might be okay with that and you're software WILL work, you run the risk of more maintenance issues and modularity problems).

Here's what a quick summary of what I've got:

   1: public QueuedEmailService : IEmailService

   2: {

   3:     public void Send(...)

   4:     {

   5:         // add to some queue

   6:     }

   7: }

8:

   9: public RealTimeEmailService : IEmailService

  10: {

  11:     public void Send(...)

  12:     {

  13:         // send right now

  14:     }

  15: }

16:

  17: public PriorityBasedEmailService : IEmailService

  18: {

  19:     public void Send(...)

  20:     {

  21:         if (priority == MailPriority.High)

  22:             // send using realtime service

  23:         else

  24:             // send using queued service

  25:     }

  26: }

27:

28:

I know many people who would look at the code above and think it is possibly overkill. I would disagree for a few reasons:

Each class does only one thing. If we have a problem with some piece of functionality it is much easier to debug. Each of the three classes above in their full implementation will fit on your screen without the need for scrolling. It's quite refreshing.
Modular design - since the application only knows that it's dealing with an IEmailService I can easily swap out implementations. For example, if we decide that we want all emails to be sent real-time, I just change how I've configured Windsor (my IoC container) and it works, because I have a concrete class which sends real-time. If you were to combine all of your code into one class, you'd have to go back and change your code and go through the pain of deployment.
Open-Closed principle - since each of the classes only have only one thing they do, I can more easily say they are closed for modification, reducing the chance that I introduce a bug later on. If you put this code into one big method, you can change the code to meet your new needs, but each change introduces the possibility of introducing a bug.

There are cases when a design can go overboard, but for the vast majority of cases this simply isn't the case. I'd rather see a solution where someone has gone too far than what I typically see in classes that do it all, with no clear responsibility.

I hope this helps augment your knowledge of SRP. If you're still a little fuzzy on SRP and what it is, let me know, I'd love to discuss it with you.