Refactoring a complex ASP.NET page code - Part I

Introduction

First, I would like to give you a simple introduction to Refactoring. Following lines are taken from this website.

What is Refactoring?

Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior. Its heart is a series of small behavior preserving transformations. Each transformation (called a 'refactoring') does little, but a sequence of transformations can produce a significant restructuring. Since each refactoring is small, it's less likely to go wrong. The system is also kept fully working after each small refactoring, reducing the chances that a system can get seriously broken during the restructuring.

I have read Martin Fowler's book about Refactoring. I strongly recommend you to read this book. It changed my vision about software. In the Refactoring book, there is a catalog of refactorings. I will not rewrite those words here. But if I have an explanation in my own words, I will give this information to you. But most of the time, I will only tell which refactoring I have used. I am quite a newbie to refactoring; therefore, if I use some names incorrectly, please bear with me. And for a refactoring to work, our code must be tested. So, refactoring and TDD work together better. But my Unit Test knowledge is not good and I needed to refactor these anyway, so there are no Unit Tests in my sample except the one which I used to launch the application and see that if it works.

Now, let me introduce my problem to you. I need an example with parameter passing in MS SQL Reporting Services since I have a problem with MS SQL Reporting Services built-in ReportViewer. After certain number of parameters, it does not work. At least in my machine. My friend Abdullah gave me this sample and told me to look at it. It is a fairly long sample with some complex code. After 04:00 midnight, I decided to write an article about refactoring it. Here it is.

You can find the original code in the gotdotnet user samples: "Multi-select ListBox sample for SQL Server Reporting Services parameters", uploaded by switter.

MyStartingSample is here:

<%@ Page Language="VB" Debug="true" 

AspCompat="true" %>
<%@ import Namespace="System.Data" %>
<%@ import Namespace="System.Data.SqlClient" %>
<script runat="server">
'**********************************************************
' COMMENTS BEFORE USING
' you'll have to add the USER ID and PWD properties
' for the data connection user and password)
' search this page for "USER ID" and "PWD"
'**********************************************************
Sub Page_Load(Sender As Object, E As EventArgs)
    if not ispostback then
        BindCategory()
    end if
end sub

Sub BindCategory()
    Dim ConnectionString As String = _
      "SERVER=(local); DATABASE=RS_TEST; USER ID=;PWD=;"
    Dim SelectCommand as string = _
      "SELECT DISTINCT m.REPORT_CATEGORY FROM RS_REPORT_PATH m"
    Dim myConnection As New SqlConnection(ConnectionString)
    Dim myCommand As New SqlDataAdapter(SelectCommand, myConnection)
    Dim ds As New DataSet() myCommand.Fill(ds) _
      dlCategory.DataSource = dsdlCategory.DataBind()
End Sub

Sub PopulateMenu(Sender As Object, E As EventArgs)
     Table4.visible=true BindMenu()
End Sub

Sub BindMenu()
    Dim ConnectionString As String = _
      "SERVER=(local); DATABASE=RS_TEST; USER ID=; PWD=;"
    Dim SelectCommand as string = _
      "SELECT m.REPORT_INDEX, m.REPORT_PATH, m.REPORT_DESCRIPTION " _
      & "FROM RS_REPORT_PATH m WHERE M.REPORT_CATEGORY='" & _
      CType(dlCategory.SelectedItem.FindControl("Button1"), _
      LinkButton).Text & "'" 
    Dim myConnection As New SqlConnection(ConnectionString)
    Dim myCommand As New SqlDataAdapter(SelectCommand, myConnection)
    Dim ds As New DataSet() myCommand.Fill(ds)    
    DataList1.DataSource = ds DataList1.DataBind()
End Sub
''HTML PART details omitted.
<Asp:DataList>
 <asp:LinkButton id="button1" forecolor="black" 
   runat="server" 
   Text='<%# DataBinder.Eval(Container.DataItem, "REPORT_CATEGORY") %>'
   CommandName="select" />
</Asp:DataList>
<Asp:DataList> 
 <asp:HyperLink runat="server" forecolor="black" 
   Text='<%# DataBinder.Eval(Container, "DataItem.REPORT_DESCRIPTION") %>'
   NavigateUrl='<%# "Parameters.aspx?REPORT_INDEX= " & _
        DataBinder.Eval(Container, "DataItem.REPORT_INDEX") %>'  
   Target="_self">
 </asp:HyperLink>
</asp:DataList>

Well, first of all, this sample does not provide any Visual Studio .NET solution.

1. I have created a solution in Visual Studio .NET and added all aspx pages to this solution. Tested it and saw that it works as it should.
2. I added this solution to source control.
3. I started to get all script blocks in aspx pages to get code-behind pages, therefore getting compile advantages. I have worked this way. I added a new page to my solution. I copy-pasted all script block code to my new page's aspx.vb (code behind) part. Then copy pasted all HTML to my new page's aspx part. I tested the code again. But it did not work as it should.

Pages came with static HTML but without data bindings. I tried to debug the code-behind. When code did not stop at breakpoints, I understood that I forgot to add event handlers. What I mean is this:

<script runat="server">
Sub Page_Load(Sender As Object, E As EventArgs)
     If Not IsPostBack Then
       BindCategory()
     End If
End Sub
</script>

works when it is in aspx part of the page. But when you have this code in aspx.vb, you have to add event handlers like this:

Private Sub Page_Load(ByVal Sender As Object, _
                      ByVal E As EventArgs) Handles  
    MyBase.Load
    If Not IsPostBack Then
        BindCategory()
    End If
End Sub

Of course, most of the time Visual Studio handles this, so I tend to forget it. Note: If you add AutoEventWire="True" to your @Page directive, you will not need to add event handlers explicitly. ASP.NET will add event handlers using common naming conventions.

After that pages started to work with code-behind. I removed old aspx pages from the solution. And I started to use new pages.

Hmm. Let us start by renaming variables to more meaningful names. This is Rename Refactoring from Fowler. Do not underestimate the power of renaming variables. We, human beings, have a very powerful mechanism for interacting with complex things; we abstract from them, name them. (I got this quote from a book but I do not remember right now which book, I will update when I remember or find it again.)

When I say table, what you understand? A wooden table, an HTML table, a System.Web.UI.HtmlControls.Table or System.Web.UI.WebControls.Table? A simple word table carries a lot of meaning with it. So, we should name our variables as we intend to use them. DataList1, Table4, button1 as variables names do not give us information about their usage. But dlAvailableReports, tblReportMenu, btnOpenReport give us more information. How do we rename our variables. I suggest you to try to rename them so that you will remember their function when you next read this code. I would like to give a Bad Practice example here. When I was maintaining a JavaScript code in my last company, I stumbled upon a JavaScript function. This function had 4 input parameters. Guess what were their names? function handleColor(var one, var second, var third, var four). With this JavaScript function, how can you tell apart between variables?

Coming back to our example: I chose to rename datalist1 as dlAvailableReports since in this DataList, "Available Reports" was its header text. This way when I look at the UI, I can easily discern which DataList is dlAvailableReports. I chose to rename button1 as btnOpenReports since that was the function of that button.

After this, rename refactorings. Code is not very readable. Hmm... what to refactor? Ahh... ConnectionString.

Sub BindCategory()
 Dim ConnectionString As String = _
   "data source=(local);initial catalog=RS_TEST;persist" & _
   " security info=False;user id=sa;pwd=;packet size=4096"
  ...
End Sub

Sub BindMenu()
 Dim ConnectionString As String = _
  "data source=(local);initial catalog=RS_TEST;persist " & _ 
  "security info=False;user id=sa;pwd=;packet size=4096"
  ...
End Sub

These two functions use the same string. So there is an opportunity for refactoring here. XP practices recommend the easiest thing that works. First thought was to use a const string in the class constructor. But I decided to use ASP.NET appSettings in web.config. I added web.config to the following lines. It seems that I do not listen to recommendations well.

<appSettings>
  <add key="ConnectionString" 
  value="data source=(localhost);initial catalog=RS_TEST;
    persist security info=False;user  id=sa;pwd=;packet size=4096" />
</appSettings>

Then, in BindCategory and BindMenu Subs, I replaced same lines with this code.

Dim ConnectionString As String = _
 ConfigurationSettings.AppSettings("ConnectionString")

After this, Fowler Refactoring recommends Inline Temp Refactoring. Which basically means replacing temp variables like ConnectionString here with function calls or expressions. Like these:

Dim ConnectionString As _
    String = ConfigurationSettings.AppSettings("ConnectionString")
Dim myConnection As New SqlConnection(ConnectionString)

to

Dim myConnection as new _
  SqlConnection(ConfigurationSettings.AppSettings("ConnectionString"))

But, I read more easily the former code, so I did not use that refactoring. One reason I like the former one is that with my monitor, I read more easily the former one but have to close some windows (Properties etc.) in VS.NET. For me, refactoring is mostly about writing code which is easily readable. After this refactoring, my BindMenu Sub looks like this:

Sub BindMenu()
    Dim ConnectionString As String = _
      ConfigurationSettings.AppSettings("ConnectionString")
    Dim SelectCommand as string = _
      "SELECT m.REPORT_INDEX, m.REPORT_PATH, m.REPORT_DESCRIPTION " _
      & "FROM RS_REPORT_PATH m WHERE M.REPORT_CATEGORY='" & _
      CType(dlCategory.SelectedItem.FindControl("btnOpenReport"), _
      LinkButton).Text  & "'"
    Dim myConnection As New SqlConnection(ConnectionString)
    Dim myCommand As New SqlDataAdapter(SelectCommand, myConnection)

    Dim ds As New DataSet()
    myCommand.Fill(ds)
    DataList1.DataSource = ds
    DataList1.DataBind()
End Sub

I find reading of this particular piece of code below difficult. Casting and using its properties:

CType(dlCategory.SelectedItem.FindControl("btnOpenReport"), _
                                                  LinkButton).Text

Let's use Extract Method Refactoring.

Private Function getSelectedItemText() As String
  Return CType(dlCategory.SelectedItem.FindControl("btnReportItem"), _
                                                          LinkButton).Text
End Function

Sub BindMenu()
  Dim ConnectionString As String = _
    ConfigurationSettings.AppSettings("ConnectionString")
  Dim SelectCommand As String = _
    "SELECT REPORT_INDEX, REPORT_PATH, REPORT_DESCRIPTION " _
    & "FROM RS_REPORT_PATH  WHERE REPORT_CATEGORY='" & _
    getSelectedItemText() & "'"
  Dim myConnection As New SqlConnection(ConnectionString)
  Dim myCommand As New SqlDataAdapter(SelectCommand, myConnection)

  Dim ds As New DataSet
  myCommand.Fill(ds)
  dlAvailableReports.DataSource = ds
  dlAvailableReports.DataBind()
End Sub

Instead of getSelectedItemText function, a ReadOnly property can be used of course. A matter of taste...

In the BindCategory Sub:

Sub BindCategory()
    Dim ConnectionString As String = _
      "data source=(localhost);initial catalog=RS_TEST;" & _ 
      "persist security info=False;user id=sa;pwd=;packet size=4096"
    Dim SelectCommand As String = _
      "SELECT DISTINCT m.REPORT_CATEGORY FROM RS_REPORT_PATH m"
    ...
End Sub

Use refactoring "Introduce Explaining Variable":

Public Const DistinctReportCategory As String = _
  "SELECT DISTINCT REPORT_CATEGORY FROM RS_REPORT_PATH"

Sub BindCategory()
    Dim ConnectionString As String = _
        ConfigurationSettings.AppSettings("ConnectionString")
    Dim SelectCommand As String = DistinctReportCategory
    ...

Here, I choose to replace the long SELECT statement with a (relatively) short variable. Now, I know which SELECT is this. It is the SELECT which gets DistinctReportCategory. And, I will not confuse this SELECT with another one. I chose not to refactor other SELECTs since I did not understand their usage at this time clearly. Making a refactoring for the sake of refactoring to that code would be counterproductive in my opinion. After this refactoring, my code looks like this:

Sub BindMenu()
    Dim ConnectionString As String = _
      ConfigurationSettings.AppSettings("ConnectionString")
    Dim SelectCommand As String = _
      "SELECT REPORT_INDEX, REPORT_PATH,REPORT_DESCRIPTION " _
      & "FROM RS_REPORT_PATH  WHERE REPORT_CATEGORY='" & _
      getSelectedItemText() & "'"
    Dim myConnection As New SqlConnection(ConnectionString)
    Dim myCommand As New SqlDataAdapter(SelectCommand, myConnection)
    Dim ds As New DataSet
    myCommand.Fill(ds)
    dlAvailableReports.DataSource = ds
    dlAvailableReports.DataBind()
End Sub

Sub BindCategory()
    Dim ConnectionString As String = _
      ConfigurationSettings.AppSettings("ConnectionString")
    Dim SelectCommand As String = DistinctReportCategory
    Dim myConnection As New SqlConnection(ConnectionString)
    Dim myCommand As New SqlDataAdapter(SelectCommand, myConnection)
    Dim ds As New DataSet
    myCommand.Fill(ds)
    dlCategory.DataSource = ds
    dlCategory.DataBind()
End Sub

hmm. You see what I see, common codes; after an "Extract Method Refactoring".

Sub BindCategory()
    Dim SelectCommand As String = DistinctReportCategory
    BindDataList(dlCategory, SelectCommand)
End Sub

Sub BindMenu()
    Dim SelectCommand As String = _
      "SELECT REPORT_INDEX, REPORT_PATH,REPORT_DESCRIPTION " _
      & "FROM RS_REPORT_PATH  WHERE REPORT_CATEGORY='" & _
      getSelectedItemText() & "'"
    BindDataList(dlAvailableReports, SelectCommand)
End Sub

Private Sub BindDataList(ByVal dl As DataList, ByVal SelectCommand As String)
    Dim ConnectionString As String = _
      ConfigurationSettings.AppSettings("ConnectionString")
    Dim myConnection As New SqlConnection(ConnectionString)
    Dim ds As New DataSet
    Dim myAdap As New SqlDataAdapter(SelectCommand, myConnection)
    myAdap.Fill(ds)
    dl.DataSource = ds
    dl.DataBind()
End Sub

After all refactoring, my code now looks like this:

Sub Page_Load(ByVal Sender As Object, ByVal E As EventArgs)
    If Not IsPostBack Then
        BindCategory()
    End If
End Sub

Sub BindCategory()
    Dim SelectCommand As String = DistinctReportCategory
    BindDataList(dlCategory, SelectCommand)
End Sub

Sub PopulateMenu(ByVal Sender As Object, ByVal E As EventArgs)
    tblReportMenu.Visible = True
    BindMenu()
End Sub

Private Function getSelectedItemText() As String
    Return CType(dlCategory.SelectedItem.FindControl("btnReportItem"), _
                                                          LinkButton).Text
End Function

Sub BindMenu()
    Dim SelectCommand As String = _
      "SELECT REPORT_INDEX, REPORT_PATH, REPORT_DESCRIPTION " _
      & "FROM RS_REPORT_PATH  WHERE REPORT_CATEGORY='" & _
      getSelectedItemText() & "'"
    BindDataList(dlAvailableReports, SelectCommand)
End Sub

Private Sub BindDataList(ByVal dl As DataList, ByVal SelectCommand As String)
    Dim ConnectionString As String = _
      ConfigurationSettings.AppSettings("ConnectionString")
    Dim myConnection As New SqlConnection(ConnectionString)
    Dim ds As New DataSet
    Dim myAdap As New SqlDataAdapter(SelectCommand, myConnection)
    myAdap.Fill(ds)
    dl.DataSource = ds
    dl.DataBind()
End Sub

As I see it. now there is no need for BindCategory.

Sub Page_Load(ByVal Sender As Object, ByVal E As EventArgs)
    If Not IsPostBack Then
        BindDataList(dlCategory, DistinctReportCategory)
    End If
End Sub

In this piece of code, it is clear that category is data bound.

What about this?

Sub PopulateMenu(ByVal Sender As Object, ByVal E As EventArgs)
    tblReportMenu.Visible = True
    BindMenu()
End Sub

Private Function getSelectedItemText() As String 
    Return CType(dlCategory.SelectedItem.FindControl("btnReportItem"), _
                                                           LinkButton).Text
End Function

Sub BindMenu()
    Dim SelectCommand As String = _
      "SELECT REPORT_INDEX, REPORT_PATH,REPORT_DESCRIPTION " _
      & "FROM RS_REPORT_PATH  WHERE REPORT_CATEGORY='" & _
      getSelectedItemText() & "'"
    BindDataList(dlAvailableReports, SelectCommand)
End Sub

In my opinion, removing PopulateMenu will lead to information loss. But, removing BindMenu() will not.

Sub PopulateMenu(ByVal Sender As Object, ByVal E As EventArgs)
    tblReportMenu.Visible = True
    Dim SelectCommand As String = _
      "SELECT REPORT_INDEX, REPORT_PATH,REPORT_DESCRIPTION " _
      & "FROM RS_REPORT_PATH  WHERE REPORT_CATEGORY='" & _
      getSelectedItemText() & "'"
    BindDataList(dlAvailableReports, SelectCommand)
End Sub

It is better now.

Lastly, I give you the code's final form.

    Protected dlCategory As System.Web.UI.WebControls.DataList
    Protected tblReportMenu As System.Web.UI.WebControls.Table
    Protected dlAvailableReports As System.Web.UI.WebControls.DataList
    Public Const DistinctReportCategory As String = _
      "SELECT DISTINCT REPORT_CATEGORY FROM RS_REPORT_PATH"

Sub Page_Load(ByVal Sender As Object, ByVal E As EventArgs)
    If Not IsPostBack Then
        BindDataList(dlCategory, DistinctReportCategory)
    End If
End Sub

Sub PopulateMenu(ByVal Sender As Object, ByVal E As EventArgs)
    tblReportMenu.Visible = True
    Dim SelectCommand As String = _
      "SELECT REPORT_INDEX, REPORT_PATH, REPORT_DESCRIPTION " _
      & "FROM RS_REPORT_PATH  WHERE REPORT_CATEGORY='" & _
      getSelectedItemText() & "'"
    BindDataList(dlAvailableReports, SelectCommand)
End Sub

Private Function getSelectedItemText() As String
    Return CType(dlCategory.SelectedItem.FindControl("btnReportItem"), _
                                                           LinkButton).Text
End Function

Private Sub BindDataList(ByVal dl As DataList, ByVal SelectCommand As String)
    Dim ConnectionString As String = _
      ConfigurationSettings.AppSettings("ConnectionString")
    Dim myConnection As New SqlConnection(ConnectionString)
    Dim ds As New DataSet
    Dim myAdap As New SqlDataAdapter(SelectCommand, myConnection)
    myAdap.Fill(ds)
    dl.DataSource = ds
    dl.DataBind()
End Sub

All of us have to do a small amount of refactoring when we have to maintain code. Or take an example from the Internet and work through it.