Design principles and testing – part 1
(Head here for an introduction)
So, to start let’s try and do a quick analysis of exactly what the OnLoad method in our page does in it’s current form.
The OnLoad method
1: protected override void OnLoad(System.EventArgs e)
2: {
3: base.OnLoad(e);
4:
5: var weatherXmlData = XElement.Load("http://www.yr.no/place/Sweden/Stockholm/Stockholm/forecast.xml");
6:
7: var today = weatherXmlData
8: .Descendants("time")
9: .FirstOrDefault();
10:
11: if(today != null)
12: {
13: Temperature.Text = today.Element("temperature").Attribute("value").Value;
14: WeatherSymbol.ImageUrl = string.Concat("~/", today.Element("symbol").Attribute("number").Value, ".png");
15: }
16: }
This method fetches some xml-data from another server, process the returned xml and then finally sets properties on some controls.
So what’s the problem with this?
As you can see this method now handles at least two different things, fetching data and formatting it. That means that when you must change either one of those implementations you have to change this method to do it. Let’s say that you want to change the formatting. You then first have to read the whole method to realize that the first part has nothing to do with formatting, just fetching. You then have to be careful that the changes you just made doesn’t affect the fetching part of the method.
Our code would be much more readable and easier to maintain of those different responsibilities were more contained. So let’s create some methods with telling names.
1: protected override void OnLoad(System.EventArgs e)
2: {
3: base.OnLoad(e);
4:
5: SetupWeather();
6: }
7:
8: private void SetupWeather()
9: {
10: var weatherData = GetWeatherData();
11: FormatWeatherDataAndSetControlValues(weatherData);
12: }
13:
14: private XElement GetWeatherData()
15: {
16: var weatherXmlData = XElement.Load("http://www.yr.no/place/Sweden/Stockholm/Stockholm/forecast.xml");
17:
18: return weatherXmlData
19: .Descendants("time")
20: .FirstOrDefault();
21: }
22:
23: private void FormatWeatherDataAndSetControlValues(XElement todaysWeatherData)
24: {
25: if (todaysWeatherData != null)
26: {
27: Temperature.Text = todaysWeatherData.Element("temperature").Attribute("value").Value;
28: WeatherSymbol.ImageUrl = string.Concat("~/", todaysWeatherData.Element("symbol").Attribute("number").Value, ".png");
29: }
30: }
This makes the process of finding what you’re after so much easier. If you, for instance, is faced with the task of changing the way the data is fetched where would your gut feeling tell you to look? A method called GetWeatherData or a method called OnLoad?
It’s also easy to imagine that our page is going to be using other services, perhaps some SetupRssFeed which would make the separation of different concerns even more important.
The single responsibility principle
What we’ve been talking about here is related to the single responsibility principle (SRP) which states “every object should have a single responsibility, and that responsibility should be entirely encapsulated by the class”. It can also be phrased that classes should have just one reason to change.
As you notice this principle talks about classes and I just refactored the code to different methods, but they still belong to the same class. Remember (and this goes for basically everything in this post and the posts to come) that these are principles and guidelines; not rules; and there’s an important difference.
Talking specifically about SRP, it’s also somewhat up to you as a developer to define what “responsibility” means to you, often this can vary depending on context.
Refactoring to comply with SRP
If we wanted to comply with SRP one approach could be the following separation. We create a data abstraction class that represents the data we’re interested in. This is returned from our weather service which behind the scenes works with a weather repository (that fetches xml) and a mapper that maps from xml to the data abstraction.
1: public class WeatherData
2: {
3: public string Temperature { get; set; }
4: public string Symbol { get; set; }
5: }
1: public class YrNoWeatherRepository
2: {
3: public XElement Load()
4: {
5: return XElement.Load("http://www.yr.no/place/Sweden/Stockholm/Stockholm/forecast.xml");
6: }
7: }
1: public class WeatherDataMapper
2: {
3: public static WeatherData FromYrNoXml(XElement weatherXmlData)
4: {
5: WeatherData weatherData = new WeatherData();
6:
7: var today = weatherXmlData
8: .Descendants("time")
9: .FirstOrDefault();
10:
11: if(today == null)
12: {
13: return weatherData;
14: }
15:
16: weatherData.Temperature = today.Element("temperature").Attribute("value").Value;
17: weatherData.Symbol = string.Concat("~/", today.Element("symbol").Attribute("number").Value, ".png");
18:
19: return weatherData;
20: }
21: }
1: public class WeatherService
2: {
3: public WeatherData GetWeatherData()
4: {
5: var repository = new YrNoWeatherRepository();
6:
7: return WeatherDataMapper.FromYrNoXml(repository.Load());
8: }
9: }
We can now rewrite the SetupWeather to work with the service, like this
1: private void SetupWeather()
2: {
3: var weatherData = new WeatherService().GetWeatherData();
4:
5: Temperature.Text = weatherData.Temperature;
6: WeatherSymbol.ImageUrl = weatherData.Symbol;
7: }
So what does all this moving and creating classes give us?
So, by the looks of it we retain the same functionality but now it’s spread out amongst a lot of classes. Besides the pros we talked about above the code, since we’ve moved it away from code behind, it’s easier to reuse (since sharing code between code behind files can be problematic). This also makes the code behind file less aware of specific implementations. It now talks to some service to get WeatherData but it doesn’t know or care about exactly how that data is fetched and formed.
While this solution might look slightly over-engineered at the moment structuring your code in this manner has other benefits as well which I’ll talk about in later posts.
Comments