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">
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
<Asp:DataList>
<asp:LinkButton id="button1" forecolor="black"
runat="server"
Text=
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.
- 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.
- I added this solution to source control.
- 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
Sub
s, 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 SELECT
s 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.
I started programming in 1991 with Amiga 68000 Assembler. I am a web and database developer proficient in different languages and databases