Imagine it’s 1805 and you’re the Emperor of Austria. You’ve commissioned a composer for a symphony. He returns after just 3 days and delivers something he came up with by playing random notes on random instruments as fast as possible.
“Dies ist nicht Musik!” you say in German. “This is not music!”
“Was?” replies the genuinely shocked composer in German. “What? These are all real musical notes on these sheets. I certainly didn’t just pick random frequencies. Did I not write parts for orchestral musical instruments? I didn’t bang on trash bins and play empty jugs.”
“There’s no structure to it. No melody, no chords, no counterpoints between instruments, no use of time signatures. Just a long run-on sequence of random notes. There’s nothing that we’ve learned about making orchestral music, or music in general, for the last 500 years. I won’t pay,” you say.
“I’ll take you to court. You asked for music and I delivered music,” threatens the composer.
(Let’s pretend this is a very democratic 1805 Austria and common citizens really can take the emporer to court and hope to win.)
“No reasonable person would consider what you’ve created music. No musical theory would hold this as an example of an expression of that theory.”
The composer stews for a moment and then realizes that he’s out of his league. He hangs his head and then looks up.
“I’ve seen symphonies performed. They’re so complex and there’s so much structure to learn. How can I even hope to do this in a reasonable time?” asks the composer.
“PRACTICE!”
A Real Life Example
I’ve seen real line-of-business web applications that are just page after page of this kind of thing:
ASPX File
<body>
<form id="form1" runat="server">
<div>
<asp:GridView ID="GridView1" runat="server" AutoGenerateColumns="false">
</asp:GridView>
</div>
</form>
</body>
Code behind
protected void Page_Load(object sender, EventArgs e)
{
if (!IsPostBack)
{
BoundField bf = new BoundField();
bf.DataField = "Title";
bf.HeaderText = "Title";
GridView1.Columns.Add(bf);
bf = new BoundField();
bf.DataField = "Year";
bf.HeaderText = "Year";
GridView1.Columns.Add(bf);
HyperLinkField hlf = new HyperLinkField();
hlf.HeaderText = "IMDB Link";
hlf.DataTextField = "ImdbUrl";
hlf.DataNavigateUrlFields = new string[] { "ImdbUrl" };
GridView1.Columns.Add(hlf);
using (ServiceReference1.MovieServiceClient mc = new ServiceReference1.MovieServiceClient())
{
var mvs = mc.GetMovies();
DataSet1.MovieDataTable mt = new DataSet1.MovieDataTable();
foreach (var mv in mvs)
{
var mr = mt.NewMovieRow();
mr.Title = mv.Title;
mr.Year = mv.Year;
mr.ImdbUrl = mv.ImdbUrl;
mt.AddMovieRow(mr);
}
GridView1.DataSource = mt;
}
GridView1.DataBind();
}
}
What? It’s C#. It compiles. It even produces the required output.
If you are thinking these things without irony and sarcasm then please, please, please keep reading to find out why they’re ironic.
(I’m actually getting angry writing this code because of the pain I endure looking at this garbage every day.)
A difference between code and music is that even though the output that the code produces is correct the underlying stuff that produced it can fail to conform to any accepted theory of structure, called design patterns, or best practices, like the SOLID principles of object oriented programming. It’s like using auto tune to correct someone who never learned to sing. You just speak into the mic and a (somewhat) pleasing sound comes out the other end.
Another difference between a song and a program is that you don’t have to add things to a song or change it after it’s been finished. The above program is fine to illustrate a point, like I’m doing, but it is not acceptable for any program that people will actually use. They’ll eventually need a change or find a bug that will require updates.
A problem with this code, but not the main one, is the obtuse variable names. What’s GridView1 supposed to represent? Oh, a list of movies. Let’s call it MovieGridView then. Mc? Oh, it’s the data service client. Let’s call it movieServiceClient. Bf used again and again to represent different things? If we really need to add columns dynamically to a grid view how about separate instances with distinct names. Mvs, mt, mr, mv? Wtf? Poor variable names make code much, much harder to maintain than it needs to be.
Here’s the same code with just the variable names updated:
ASPX File
<body>
<form id="MoviesForm" runat="server">
<div>
<asp:GridView ID="MoviesGridView" runat="server" AutoGenerateColumns="false">
</asp:GridView>
</div>
</form>
</body>
Code behind
protected void Page_Load(object sender, EventArgs e)
{
if (!IsPostBack)
{
BoundField titleField = new BoundField();
titleField.DataField = "Title";
titleField.HeaderText = "Title";
MoviesGridView.Columns.Add(titleField);
BoundField yearfield = new BoundField();
yearfield.DataField = "Year";
yearfield.HeaderText = "Year";
MoviesGridView.Columns.Add(yearfield);
HyperLinkField imdbUrlField = new HyperLinkField();
imdbUrlField.HeaderText = "IMDB Link";
imdbUrlField.DataTextField = "ImdbUrl";
imdbUrlField.DataNavigateUrlFields = new string[] { "ImdbUrl" };
MoviesGridView.Columns.Add(imdbUrlField);
using (ServiceReference1.MovieServiceClient movieServiceClient = new ServiceReference1.MovieServiceClient())
{
var moviesFromService = movieServiceClient.GetMovies();
MovieDataSet.MovieDataTable movieTable = new MovieDataSet.MovieDataTable();
foreach (var movie in moviesFromService)
{
var movieRow = movieTable.NewMovieRow();
movieRow.Title = movie.Title;
movieRow.Year = movie.Year;
movieRow.ImdbUrl = movie.ImdbUrl;
movieTable.AddMovieRow(movieRow);
}
MoviesGridView.DataSource = movieTable;
}
MoviesGridView.DataBind();
}
}
Now other folks’ brains don’t need to work nearly as hard to understand the intent. We’re still using a DataSet to store non-SQL data internally, which is depraved, but I’m just mirroring what I see every day. I also didn’t bother to re-add the service reference with a better name.
But here’s the biggest problem with this code: no separation of concerns. I’m talking about the S in SOLID, the Single Responsibility Principle. This states that a class should be responsible for only 1 aspect of the application. “A class should have 1 and only 1 reason to change”.
In general, a “data over textboxes” app like the one above has 3 main aspects:
- moving data to and from a source
- formatting data/interpreting user input
- presenting data.
A single class currently handles all 3 of these aspects, the code behind. Oh, you didn’t realize that it’s a class and should be treated like any other? The declaration “public partial class” before the name of the page in the code behind wasn’t a clue?
Multiple aspects will require a change to this 1 class – a change in the data source, a change in the internal data model (the DataSet), a change in formatting of an existing field, or a requirement to get updated data some time after the initial page load.
There should be at least 3 more classes:
- One with a method named something like GetDataForInitialPageLoad that takes the page as an argument. This will be called from within the if (!IsPostBack) block and pass the page in using the “this” keyword. The method will use the next class to get data and wire it to the page’s GridView. This class houses business logic.
- One with a method named something like GetMovies that retrieves data from the service and uses the next class to convert it from the service’s format to a DataTable. GetDataForInitalPageLoad would call this method.
- One with a method named something like Convert that takes the service’s output as input and returns a DataTable.
In addition, the GridView fields being added in code should be specified in the ASPX file.
Now, if any 1 of the aspects I mentioned above change, only 1 class needs to change and you know no other aspect of the app has changed. This cuts down on the testing needed and dramatically lowers the odds of inadvertently introducing a change in an unrelated aspect. It also sets you up to follow the rest of the SOLID principles and allows unit testing.
My favorite pattern for separating concerns when developing WebForms is Model-View-Presenter. The ASPX page and code behind are the View, some business logic class specific to the view is the Presenter, and the DataSet, converters and service comprise the Model. The refactored version of this app using MVP follows below. You’ll see that there’s not much more code, but there are more classes and interfaces. The biggest leap to make is grasping the concept of the Single Responsibility Principle and the way the presenter and view share things.
But after you grasp it, you have to try implementing it again and again until it becomes second nature. Learn the VS2010 shortcut keys for creating classes and code snippets for adding properties. It’s a pattern; it’s not different every time. Once you learn the pattern it will flow from your brain to your code just as quickly as mvs=mc.GetMovies().
Conclusion
Whether you’re the Emperor of Austria in 1805 commissioning a symphony or a private citizen in 2011 commissioning a piece of art you wouldn’t accept something that didn’t follow some well known precepts of the medium in which your artist works. No true artist would try to deliver something that they didn’t feel was an expression of some theory which they’d studied, if only a little. The more studied and practiced the artist the quicker they can produce works that conform to that theory. The top notch ones even expand on the theory.
So why is is that people who call themselves programmers think it totally acceptable to produce code that meets requirements but follow none of the best practices determined over the last 40 years?
I once sat in a class where the instructor went over the Model-View-ViewModel pattern in Silverlight and how, like any pattern that separates concerns, it leads to much more maintainable code. A “developer” put up his hand and asked “Yeah, but what if you’re trying to meet a deadline?” The instructor replied, “The more often you implement this the faster you’ll get".”
In other words, PRACTICE!
Code Plz
The ASPX File
<%@ Page Language="C#" AutoEventWireup="true" CodeBehind="NotTheWayItsDone.aspx.cs"
Inherits="Wrong.NotTheWayItsDone" %>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head runat="server">
<title></title>
</head>
<body>
<form id="MoviesForm" runat="server">
<div>
<asp:GridView ID="MoviesGridView" runat="server" AutoGenerateColumns="False">
<Columns>
<asp:BoundField DataField="Title" HeaderText="Title" />
<asp:BoundField DataField="Year" HeaderText="Year" />
<asp:HyperLinkField DataNavigateUrlFields="ImdbUrl"
DataTextField="ImdbUrl" HeaderText="IMDB Link" />
</Columns>
</asp:GridView>
</div>
</form>
</body>
</html>
Reasoning
The ASPX file is all about display. Unless you’re adding columns dynamically to the GridView, specify here which data go in which fields and how they should be formatted. Wire up client-side JavaScript calls here. Use HTML class attributes to let CSS do its thing. You get a pretty good preview of what things will look like. You’ll go from this:
to this:
You can also start splitting the work between developers without everyone working on the same file.
Code behind
using System;
using System.Collections.Generic;
using System.Linq;
using System.Web.UI;
using Wrong.ServiceReference1;
namespace Wrong
{
/// <summary>
/// Really this is a page that displays a list of movies
/// </summary>
public partial class NotTheWayItsDone : Page, IMoviesView
{
/// <summary>
/// Class that contains business logic
/// </summary>
private readonly MoviesPresenter presenter;
/// <summary>
/// Initializes a new instance of the NotTheWayItsDone class.
/// </summary>
public NotTheWayItsDone()
{
// Pass this page instance to the presenter. Specify the IMovieService implementation to use, as per Inversion
// of Control principle.
// The presenter sees only the items defined in IMoviesView
this.presenter = new MoviesPresenter(this, new MovieServiceClient());
}
/// <summary>
/// Handles the Load event of the Page control.
/// </summary>
/// <param name="sender">The source of the event.</param>
/// <param name="e">The <see cref="System.EventArgs"/> instance containing the event data.</param>
protected void Page_Load(object sender, EventArgs e)
{
if (!IsPostBack)
{
this.presenter.OnViewInitialized();
}
}
/// <summary>
/// Populates the movies onto the view.
/// </summary>
/// <param name="movies">The movies.</param>
public void SetMovies(MovieDataSet.MovieDataTable movies)
{
this.MoviesGridView.DataSource = movies;
}
}
}
Reasoning
The code behind file’s job is to react to events from the user and communicate data to and from the Presenter, which implements business logic. That’s all. No retrieving data. No left hand-right hand code transforming data from one format to another. No adding visual elements that don’t appear conditionally.
It shares information with the Presenter by implementing an interface that the presenter knows about. In this case, a method that allows the Presenter to hand the GridView some data, and we’ve exposed Page.DataBind.
The presenter doesn’t know that a GridView will receive the data. It’s the View’s job to present the data to the user properly. We could change the View to use a ListView and the presenter would be none the wiser.
We’ve exposed DataBind so that the Presenter can let the View know when it’s time to bind. This is useful if you’re not using DataSource-type objects to receive data in your view, which would normally detect changes to source data and call DataBind for you.
Another class should implement business logic, like “when save is clicked, gather entered data and put it in the repository.”
The View Interface
using System;
using System.Collections.Generic;
using System.Linq;
namespace Wrong
{
public interface IMoviesView
{
/// <summary>
/// Populates the movies onto the view.
/// </summary>
/// <param name="movies">The movies.</param>
void SetMovies(MovieDataSet.MovieDataTable movies);
/// <summary>
/// Causes the view to bind data-bound controls to their source data.
/// </summary>
void DataBind();
}
}
Reasoning
The documentation in the interface and the code behind reasoning should suffice.
The Presenter
using System;
using System.Collections.Generic;
using System.Linq;
using Wrong.ServiceReference1;
namespace Wrong
{
/// <summary>
/// Contains business logic for the movies view (page)
/// </summary>
public class MoviesPresenter
{
/// <summary>
/// Initializes a new instance of the <see cref="MoviesPresenter"/> class.
/// </summary>
/// <param name="view">The view.</param>
/// <param name="movieService">The movie service.</param>
public MoviesPresenter(IMoviesView view, IMovieService movieService)
{
this.View = view;
this.MovieService = movieService;
}
/// <summary>
/// The movie service
/// </summary>
protected IMovieService MovieService { get; private set; }
/// <summary>
/// Gets or sets the view.
/// </summary>
/// <value>
/// The view.
/// </value>
protected IMoviesView View { get; set; }
/// <summary>
/// Called by the view when it is first loaded.
/// </summary>
public void OnViewInitialized()
{
var moviesFromService = this.MovieService.GetMovies();
MovieToMovieDataSetConverter converter = new MovieToMovieDataSetConverter();
using (MovieDataSet.MovieDataTable movieTable = converter.ConvertAllFrom(moviesFromService))
{
this.View.SetMovies(movieTable);
}
this.View.DataBind();
}
/// <summary>
/// Releases unmanaged resources and performs other cleanup operations before the
/// <see cref="MoviesPresenter"/> is reclaimed by garbage collection.
/// </summary>
~MoviesPresenter()
{
IDisposable moviesServiceAsDisposable = this.MovieService as IDisposable;
if (moviesServiceAsDisposable != null)
{
moviesServiceAsDisposable.Dispose();
}
}
}
}
Reasoning
This is where all the action is.
The constructer requires an instance of something that implements IMovieView. Typically this is the page, but for unit testing purposes you could pass in an object that implements the same interface. It also needs the movie client implementation specified to it. (WCF ) This is an example of the Inversion of Control principle.
In the destructor we need to check if the movie client requires disposing. The real WCF client does, but any pretend versions used for unit testing doesn’t necessarily.
The Converter
using System;
using System.Collections.Generic;
using System.Linq;
using Wrong.ServiceReference1;
namespace Wrong
{
class MovieToMovieDataSetConverter
{
/// <summary>
/// Converts all from.
/// </summary>
/// <param name="moviesFromService">The movies from service.</param>
/// <returns>A MovieDataTable populated with the data from moviesFromService</returns>
public MovieDataSet.MovieDataTable ConvertAllFrom(Movie[] moviesFromService)
{
MovieDataSet.MovieDataTable result = new MovieDataSet.MovieDataTable();
foreach (Movie movie in moviesFromService)
{
result.AddMovieRow(this.ConvertFrom(movie, result));
}
return result;
}
/// <summary>
/// Converts from.
/// </summary>
/// <param name="movie">The movie.</param>
/// <param name="movieTable">The result.</param>
/// <returns>A MovieRow based on the movieTable parameter and populated with the data from the movie parameter</returns>
public MovieDataSet.MovieRow ConvertFrom(Movie movie, MovieDataSet.MovieDataTable movieTable)
{
var movieRow = movieTable.NewMovieRow();
movieRow.Title = movie.Title;
movieRow.Year = movie.Year;
movieRow.ImdbUrl = movie.ImdbUrl;
return movieRow;
}
}
}
Reasoning
This class’s only function is to populate a DataTable with incoming data. If the incoming data type or the DataTable changes then only this class needs to change.
