1.Refactoring Code: A Programmer's
Challenge
Today I encountered a question on the ASP.NET Forum that I got
quite
excited about, "How do I refactor this?" Refactoring is a big part of
our daily coding practices and I was curious as to what other
programmers would come up with. Here is the basic question re-worded a
little bit for brevity:
I have a master page with a control on it. However, on
certain
pages I don't want a control to be shown. At first it was just one or
two pages, and my approach worked just fine. However, the number of
pages I need to hide this control on has grown. Can anyone offer some
advice on how to refactor this code?
This was his code:
string pagename =
Request.ServerVariables["PATH_INFO"].ToString().Trim().ToLower();
if
(
pagename == "/index.aspx" ||
pagename == "/sitemap.aspx" ||
pagename == "/404.aspx" ||
pagename == "/status.aspx" ||
pagename == "/online_help.aspx"
)
{
SomePanel.Visible = false;
}
The original poster realized that his code wasn't maintainable-a classic reason to refactor: Keep your code clean, readable, and maintainable.
The interesting thing about refactoring is that there is no de-facto way to improve this code-or any refactoring for that matter. Refactoring has a lot of patterns that have been used over the years, but how that pattern is implemented, and evolves, is a discussion that could go on forever. The key is: keep the code simple and keep the changes small. If we really wanted to, we could write over-the-top code; put these pages in a database table; and cache the table for performance. But if you refactor often enough the changes should always be incremental.
Here's something else to consider: the 'if' statement will
keep
getting larger and larger every time a page is added. If I was
implementing this I might have started in the same place. I wouldn't
have needed this control visible for this page, but then I might not
have needed it for the next one either, or the next one. The original
code is not bad-it just doesn't meet the new criteria.
My suggestion is to remove the OR chain and store the page names in a
local so one can just keep adding to a string array, and then check if
it exists in the string array. This simple refactoring adds a little
clarity to the code, but it's not exactly a work of art:
string[] pagesToHideElement = new[]
{
"/index.aspx",
"/sitemap.aspx",
"/404.aspx",
"/status.aspx",
"/online_help.aspx",
};
string pagename =
Request.ServerVariables["PATH_INFO"].ToString().Trim().ToLower();
if
(Array.Exists(pagesToHideElement, s => s == pagename))
{
SomePanel.Visible = false;
}
We have isolated one of our problems: the growing list of pages we want
to exclude. The developer can then keep adding to the array. This also
provides flexibility we didn't have before. We can now move our page
list anywhere we like, like a new class and method. This is a very
simple refactoring, but what else can we do? While the original person
didn't think of this, this code isn't easy to test without a full-blown
integration test.
Not to derail our refactoring discussion, but let's talk briefly about
testing and how it applies to refactoring. They go hand-in-hand,
especially to an Agile developer. Refactoring is a necessary part of
improving code, but how do you know that everything will still work?
Many who oppose refactoring don't write tests simply because they have
no proof that a refactoring didn't break the code. And refactoring
isn't always easy, especially when dealing with legacy code. Most Agile
developers are familiar with the "Red-Green-Refactor" motto:
The diagram varies, but nevertheless it always says the same
thing
1. Write a failing test first (That's a core Test Driven Development
value)
2. Make the test pass by implementing the required code
3. See the test pass
4. Refactor
5. Make sure you are still green
6. Improve test so that it is red again
It's the fifth component that makes testing and refactoring go together
well. Peace of mind comes with knowing that you didn't break anything,
assuming you have good testing practices-but of course you
do!
Back to refactoring. How is this
code refactoring
tested without actually seeing if it shows the correct content or not?
MVP or the MVC pattern are a good idea, but let's start small. A
pattern that I am fond of for these simple things is a decider class.
The benefits of this pattern will become clear shortly.
This sort of decision-making logic doesn't really belong in the
presentation layer (UI layer). It can be deferred to a decider. The
decider is exactly what it sounds like-a class that decides if the
panel should be visible or not. So our decider can have a method on it
that takes a page name, and return a Boolean value if the panel should
be displayed. Simple enough, right? At this point we are extracting our
logic out of the page and into a new class.
public class DisplayPanelDecider
{
public bool ShouldDisplayPanelOnPage(string pageName)
{
string[] pagesToHideElement = new[]
{
"/index.aspx",
"/sitemap.aspx",
"/404.aspx",
"/status.aspx",
"/online_help.aspx",
};
return !Array.Exists(pagesToHideElement, s => s ==
pageName);
}
}
So now we have a piece of logic that we can test very easily. All that
the ASPX page would have to do is create an instance and hand in the
page name, but the core decision logic can be tested without any need
for a browser.
This gives us a new power through. Now that we have this decision
making logic in its own method, we can make our method do whatever we
like. If, in a year's time, this no longer feasible, we can alter our
method to pull the page names from an XML file or a database. We'll
look at that later.
A quick recap of what we did:
- We changed the functionality to give us a bit more functionality and moved from a large OR operation to a more dynamic approach.
- We extracted our logic out to a new class.
- Two simple changes...and the benefits are huge!
- We can now test this logic very easily through our favorite testing framework. XUnit MSTest, etc. (I refer to XUnit as "Any of the testing frameworks, like NUnit, MbUnit, etc. Not the actual framework called "xUnit").
- Our logic extraction keeps the decision making out of the ASPX code behind. This keeps our ASPX code behind focused on the page, not buried in a code behind.
- We have the freedom to change what this method does without affecting the page. In the future, we can make "ShouldDisplayPanelOnPage" do whatever we like.
- Further changes to this class can be tested TDD style so we can feel confident that future refactorings won't break functionality.
Next we'll look at dependency injection and what happens when this decision making logic gets complex
source:http://www.thycotic.com/refactoring-code-a-programmers-challenge
