This thing is huge, refactoring
I thought I'd give some insight in the huge method I was telling you about in my previous post, and rant some more about software development in general. The code samples used have been edited so that they won't give information about my current assignment.
The method gets an XML document as input and needs to store the information into a couple of data stores, all using Sql server stored procedures. At some point you need to retrieve the data from the XML and create SqlParameter objects. In total, the method needs to supply over 80 Sql parameters for the various stored procedures. It is done using the following code:
Dim xmlValue As XmlNode = xml.XPath(ConfigurationService.GetConfigurationSetting("ClientId"))
sqlParameter = New sqlParameter("@ClientId", SqlDbType.VarChar)
sqlParameter.Direction = ParameterDirection.Input
sqlParameter.Value = xmlValue.InnerText
That explains partly why the method has about 1900 lines of code. The same code is copied over and over again just to get data from the XML and create a SqlParameter object.
What if I create a method to get the value from the XML document?
Private Function GetXmlValue(ByVal xmlAanvraag As XmlData, ByVal xpathName As String) As Object
Dim node As XmlNode = xmlAanvraag.XPath(ConfigurationService.GetConfigurationSetting(xpathName))
If (Not node Is Nothing) Then
And we need a method to create a SqlParameter object which we can add to the command object:
Private Function GetParameter(ByVal parameterName As String, ByVal dbType As SqlDbType, ByVal value As Object, Optional ByVal direction As ParameterDirection = ParameterDirection.Input) As SqlParameter
Dim newParameter As SqlParameter = New SqlParameter(parameterName, dbType)
newParameter.Direction = direction
newParameter.Value = value
By creating these two methods I can do this:
GetParameter("@ClientId", SqlDbType.VarChar, _
The line is split up over three lines, to make clear what is happening. But in the actual code this will be one line. I've done some estimates and only this action will reduce the number of lines with 800. Making it easier to read, better to maintain.
Another thing is that the method calls a number of stored procedures. If each stored procedure would be handled in a separate method, than the methods themselves would be really simple to read and a lot smaller than 1900 lines...
I ran a metric analysis on the code using a Reflector Add-in I found here. The Cyclomatic Complexity for this particular method is 140! Now have a look at this table:
|1 - 10
||Simple, without much risk|
|11 - 20
||More complex, moderate risk|
|20 - 50
||Complex, high risk program |
|Greater than 50
||Untestable program (very high risk)|
This table displays value ranges for Cyclomatic complexity and their meaning, the source of which can be found here. Most sources
I've written a proposal to the technical lead here and will leave it at that. This is obviously an action that needs a thorough review once you've made all the changes. And as far as I'm concerned, regression tests after such a major change are mandatory. So this action will need to be carefully planned.
Why won't developers run FxCop on their code? Why don't they try and split the code into smaller, readable, understandable methods? Every developer I know is talking about OO development and yet every time I'm assigned to a new project and do a review, I find huge methods as the ones above. If you write methods with 1900 lines, then you must have missed the whole concept of modular development. OO development is just a term you use, not something you practice. Methods of that size are prone to errors. Try fixing a bug in a method this size, or even try debugging that method! You take an enormous risk of introducing new ones.
Oh, and before people start telling me that this is because of the class being written in VB.Net. That's not the reason for code like this. I've seen it done in C# code as well. It's not the language that is responsible for s**t like this, it's the (lack of) development skills by the developer.
There, I've said it. It's of my chest. Back to work.