Refactoring with CodeRush Xpress
One of the first things I do with a new install of Visual Studio(VS) is I head over to DevExpress and download/install CodeRush Xpress. CodeRush Xpress is a free refactoring tool from DevExpress for VS 2010 or older(They stopped offering the free version for VS 2012 or newer so now it looks like the first thing I'll be doing with a new install of Visual Studio is beg my boss for a CodeRush license).
If you are not familiar with refactoring and you are interested in being a better coder you should read Martin Fowler's book: Refactoring: Improving the Design of Existing Code (Addison-Wesley Object Technology Series).
In it the authors Martin Fowler, Kent Beck, John Brant, William Opdyke and Don Roberts give very practical advice on improving the design of code.
Along with improving the design of code, refactoring can improve the readability of code. My favorite refactoring pattern for this is Extract Method(EM). When you come across a really long method or find that in a moment of coding frenzie you have created one yourself, it's time to do some EM'ing to make things more readable for the next person that has to look at your code(which may be yourself in which case you will hear the thanks first hand from yourself).
For example take this code I found on an open source project:
According to Visual Studio Code Metrics the Maintainability Index of UpdateHintPath is 16. This is not good.
The first thing you should do before undertaking any refactoring is make sure the code is covered by some automated unit test. If it isn't then you should apply one or more of the techniques outlined in Michael Feathers awesome book Working Effectively with Legacy Code. Once the code to be refactored is under test and all tests are green then we can proceed.
For the Extract Method refactoring with CodeRush Xpress we simply highlight the code we want to extract to it's own method and press the ctrl+` key. In this case we will highlight the first part of the big "If" statement:
Then make sure Extract Method is selected and press Enter.
Next use the Up and Down arrows to move the red line to where you want the new method code to be put and hit Enter.
CodeRush takes a guess at the extracted method name based on a nearby comment which can be bang on sometimes and all you would do is hit Enter and be done, but in this case I would like to change it to something that better describes what the new method is doing UpdateHintPathForDotNetTwo.
After changing the name and hitting Enter the new method creation is completed and you will notice that Coderush also took a best guess as to what needs to be passed into the new method, more on this later. To get back to the original method all that needs to be done is press the Esc key.
And there we can see our new method call we just created. Then I repeated the process for the Else statement and called it UpdateHintPathForDotNetOne.
Now we have quite a few parameters that are being passed in unnecessarily to the new methods so we clean those up and here is the final refactored method:
Visual Studio Code Metrics is now reporting that the Maintainability Index of the new UpdateHintPath is at a respectable 50. Finally remember to run the unit tests to make sure they are still all green.
In it the authors Martin Fowler, Kent Beck, John Brant, William Opdyke and Don Roberts give very practical advice on improving the design of code.Along with improving the design of code, refactoring can improve the readability of code. My favorite refactoring pattern for this is Extract Method(EM). When you come across a really long method or find that in a moment of coding frenzie you have created one yourself, it's time to do some EM'ing to make things more readable for the next person that has to look at your code(which may be yourself in which case you will hear the thanks first hand from yourself).
For example take this code I found on an open source project:
Private Function UpdateHintPath(ByVal projfile As String) As Boolean Dim xmldoc As XmlDocument Dim xnl As XmlNodeList Dim xelement As XmlElement Dim hintpathAttr As String = String.Empty Dim newHintPathAttr As String Dim name As String = String.Empty Dim assmbname As String = String.Empty Dim refinclude As String Dim projGUID As String = String.Empty Dim blDoSave As Boolean = False Dim blUpdateHintPathOk As Boolean = True Dim elementToRemove As New Hashtable Dim attrToRemove As New ArrayList Dim ien As IEnumerator File.SetAttributes(projfile, FileAttributes.Normal) xmldoc = New XmlDocument xmldoc.Load(projfile) If GetDotNetVersion(projfile) = DOTNETVERSION.TWO Then 'Get the namespace URI of the projects Dim projectroot As XmlNode = xmldoc.ChildNodes(0) If projectroot.Name.Equals("xml") Then projectroot = xmldoc.ChildNodes(1) End If Dim projectNameSpaceURI As String = CType(projectroot, System.Xml.XmlNode).NamespaceURI Dim nsmgr As XmlNamespaceManager = New XmlNamespaceManager(xmldoc.NameTable) nsmgr.AddNamespace("proj", projectNameSpaceURI) 'Remove all project references - We need to remove the project reference becuase as UBuild build at project level and not sln 'if one of the referred project is not already build but is a dependent of the main project being built it will cause the build to fail 'So we take out the project reference. xnl = xmldoc.GetElementsByTagName("ItemGroup") For Each xelement In xnl If xelement.HasChildNodes Then Dim childNodeList As XmlNodeList = xelement.ChildNodes For Each _child As XmlNode In childNodeList If _child.NodeType = XmlNodeType.Comment Then Continue For End If Dim child As XmlElement = DirectCast(_child, XmlElement) If child.Name.Equals("ProjectReference") Then If Not elementToRemove.Contains(child) Then elementToRemove.Add(child, xelement) End If If child.HasChildNodes Then If child.GetElementsByTagName("Project").Count = 1 Then projGUID = child.GetElementsByTagName("Project").Item(0).InnerText End If If child.GetElementsByTagName("Name").Count = 1 Then assmbname = child.GetElementsByTagName("Name").Item(0).InnerText End If End If 'assmbname = child.ChildNodes(0).InnerText 'projGUID = child.ChildNodes(1).InnerText Dim newelement As XmlElement = xmldoc.CreateElement("Reference", projectNameSpaceURI) Dim xmlName As XmlElement = xmldoc.CreateElement("Name", projectNameSpaceURI) Dim xmlHintPath As XmlElement = xmldoc.CreateElement("HintPath", projectNameSpaceURI) xmlName.InnerText = CType(htProjGuidAssmb(projGUID), String) xmlHintPath.InnerText = buildFileLocation & CType(htProjGuidAssmb(projGUID), String) & ".dll" newelement.SetAttribute("Include", CType(htProjGuidAssmb(projGUID), String)) newelement.AppendChild(xmlName) newelement.AppendChild(xmlHintPath) xelement.AppendChild(newelement) xmldoc.Save(projfile) If Not attrToRemove.Contains(newelement) Then attrToRemove.Add(newelement) End If End If '//If child.Name.Equals("ProjectReference") 'Find <Reference> element is present without any child node 'Remove if it is of type <Reference Include="[AssemblyName], Version=1.0.915.2008, Culture=neutral, processorArchitecture=MSIL" /> If child.Name.Equals("Reference") And child.HasAttribute("Include") And Not child.HasChildNodes Then refinclude = child.GetAttribute("Include") If InStr(refinclude, ",") Then assmbname = child.GetAttribute("Include").Split(","c)(0) If Not ToBeExcluded(assmbname) Then If Not elementToRemove.Contains(child) Then elementToRemove.Add(child, xelement) End If Dim newelement As XmlElement = xmldoc.CreateElement("Reference", projectNameSpaceURI) Dim xmlName As XmlElement = xmldoc.CreateElement("Name", projectNameSpaceURI) Dim xmlHintPath As XmlElement = xmldoc.CreateElement("HintPath", projectNameSpaceURI) xmlName.InnerText = assmbname xmlHintPath.InnerText = assmbname & ".dll" newelement.SetAttribute("Include", assmbname) newelement.AppendChild(xmlName) newelement.AppendChild(xmlHintPath) xelement.AppendChild(newelement) xmldoc.Save(projfile) If Not attrToRemove.Contains(newelement) Then attrToRemove.Add(newelement) End If End If Else refinclude = String.Empty End If '// If InStr(refinclude, ",") End If '//If child.Name.Equals("Reference") Next End If Next 'If there are project references found, then lets remove them now If elementToRemove.Count > 0 Then ien = elementToRemove.Keys.GetEnumerator While ien.MoveNext Dim _childElement As XmlElement = CType(ien.Current, XmlElement) Dim _parentElement As XmlElement = (CType(elementToRemove(ien.Current), XmlElement)) _parentElement.RemoveChild(_childElement) End While xmldoc.Save(projfile) End If 'Now update all the hint paths xnl = xmldoc.GetElementsByTagName("Reference") For Each xelement In xnl refinclude = "" assmbname = "" If xelement.HasChildNodes Then If xelement.HasAttribute("Include") Then refinclude = xelement.GetAttribute("Include") If InStr(refinclude, ",") Then assmbname = refinclude.Split(","c)(0) ElseIf ProjectsToBuildList.Contains(refinclude & ".dll") Then assmbname = refinclude ElseIf IsProxy(refinclude & ".dll") Then assmbname = refinclude End If End If 'check if there is a <HintPath> element present Dim hintpathnode As XmlNode = Nothing For i As Integer = 0 To xelement.ChildNodes.Count - 1 If String.Compare(xelement.ChildNodes(i).Name, "HintPath") = 0 Then hintpathnode = xelement.ChildNodes(i) Exit For End If Next If ((hintpathnode Is Nothing) And Not ToBeExcluded(assmbname) And assmbname.Length > 0) Then 'TODO: Add an <HintPath> element hintpathnode = xmldoc.CreateElement("HintPath", "http://schemas.microsoft.com/developer/msbuild/2003") hintpathnode.InnerText = assmbname xelement.AppendChild(hintpathnode) End If Dim childNodeList As XmlNodeList = xelement.ChildNodes For Each child As XmlElement In childNodeList If child.Name.Equals("Name") Then assmbname = child.InnerText End If If child.Name.Equals("HintPath") Then hintpathAttr = child.InnerText End If If Not assmbname Is Nothing And assmbname.Length > 0 And ToBeExcluded(assmbname) And IsProxy(assmbname & ".dll") Then newHintPathAttr = buildFileLocation & assmbname & ".dll" If proxiesList.Contains(assmbname) And Not File.Exists(newHintPathAttr) Then File.Copy(CType(proxiesList(assmbname), String), newHintPathAttr, True) End If If child.Name.Equals("HintPath") Then child.InnerText = buildFileLocation & assmbname & ".dll" End If blDoSave = True End If If child.Name.Equals("HintPath") And Not assmbname Is Nothing And assmbname.Length > 0 And Not ToBeExcluded(assmbname) And Not hintpathAttr Is Nothing AndAlso hintpathAttr.Length > 0 Then newHintPathAttr = buildFileLocation & assmbname & ".dll" If Not ProjectsToBuildList.Contains(assmbname & ".dll") And Not File.Exists(newHintPathAttr) And IsProxy(assmbname & ".dll") Then If proxiesList.Contains(assmbname & ".dll") Then File.Copy(CType(proxiesList(assmbname & ".dll"), String), newHintPathAttr, True) End If End If child.InnerText = buildFileLocation & assmbname & ".dll" blDoSave = True End If Next '// child In childNodeList End If '//if element has childnodes Next '//next <Reference> element Else 'DOTNET_VER = 1.0 xnl = xmldoc.SelectNodes("//References/Reference") For Each xelement In xnl If xelement.HasAttribute("Name") Then name = xelement.Attributes.GetNamedItem("Name").Value End If 'If proxyAliasList.Contains(name) Then ' If xelement.HasAttribute("Guid") And _ ' xelement.HasAttribute("VersionMajor") And _ ' xelement.HasAttribute("VersionMinor") And _ ' xelement.HasAttribute("Lcid") And _ ' xelement.HasAttribute("WrapperTool") Then ' xelement.RemoveAttribute("Guid") ' xelement.RemoveAttribute("VersionMajor") ' xelement.RemoveAttribute("VersionMinor") ' xelement.RemoveAttribute("Lcid") ' xelement.RemoveAttribute("WrapperTool") ' xelement.SetAttribute("AssemblyName", CType(proxyAliasList(name), String)) ' xelement.SetAttribute("HintPath", buildFileLocation & CType(proxyAliasList(name), String) + ".dll") ' blDoSave = True ' End If 'End If 'Remove project references If xelement.HasAttribute("Project") Then xelement.RemoveAttribute("Project") xelement.RemoveAttribute("Package") If BuildListMapProjToAssmbName.Contains(projfile) Then xelement.SetAttribute("AssemblyName", name) xelement.SetAttribute("HintPath", buildFileLocation & name & ".dll") blDoSave = True End If End If If xelement.HasAttribute("AssemblyName") Then assmbname = xelement.Attributes.GetNamedItem("AssemblyName").Value End If If xelement.HasAttribute("HintPath") Then hintpathAttr = xelement.Attributes.GetNamedItem("HintPath").Value End If If ToBeExcluded(assmbname) And IsProxy(assmbname & ".dll") Then newHintPathAttr = buildFileLocation & assmbname & ".dll" If proxiesList.Contains(assmbname & ".dll") And Not File.Exists(newHintPathAttr) Then File.Copy(CType(proxiesList(assmbname & ".dll"), String), newHintPathAttr, True) End If xelement.SetAttribute("HintPath", buildFileLocation & assmbname & ".dll") blDoSave = True End If If Not assmbname Is Nothing And Not ToBeExcluded(assmbname) And Not hintpathAttr Is Nothing AndAlso hintpathAttr.Length > 0 Then newHintPathAttr = buildFileLocation & assmbname & ".dll" If Not ProjectsToBuildList.Contains(assmbname & ".dll") And Not File.Exists(newHintPathAttr) And IsProxy(assmbname & ".dll") Then If proxiesList.Contains(assmbname & ".dll") Then File.Copy(CType(proxiesList(assmbname & ".dll"), String), newHintPathAttr, True) End If End If xelement.SetAttribute("HintPath", buildFileLocation & assmbname & ".dll") blDoSave = True End If Next End If '//check DOTNET_VER Try If blDoSave Then xmldoc.Save(projfile) End If Catch ex As Exception If dispXMLoutput Then Output("<message loc=""updatehintpath""><![CDATA[ERROR while updating HintPath ! Failed to save : " + projfile + "]]> </message>") Output("<message loc=""updatehintpath""><![CDATA[" + ex.Message + "]]> </message>") Else Output(vbCrLf + "Error while updating HintPath ! Failed to save : " + projfile + vbCrLf + ex.Message, ConsoleTools.ConsoleColor.red) End If WriteLog(vbCrLf + "Error while updating HintPath ! Failed to save : " + projfile + vbCrLf + ex.Message) blUpdateHintPathOk = False Throw ex Finally xmldoc = Nothing End Try File.SetAttributes(projfile, FileAttributes.ReadOnly) Return blUpdateHintPathOk End Function
According to Visual Studio Code Metrics the Maintainability Index of UpdateHintPath is 16. This is not good.
Once the code to be refactored is under test and all tests are green then we can proceed.For the Extract Method refactoring with CodeRush Xpress we simply highlight the code we want to extract to it's own method and press the ctrl+` key. In this case we will highlight the first part of the big "If" statement:
If GetDotNetVersion(projfile) = DOTNETVERSION.TWO ThenHit ctrl+` and we get:
Then make sure Extract Method is selected and press Enter.
Next use the Up and Down arrows to move the red line to where you want the new method code to be put and hit Enter.
CodeRush takes a guess at the extracted method name based on a nearby comment which can be bang on sometimes and all you would do is hit Enter and be done, but in this case I would like to change it to something that better describes what the new method is doing UpdateHintPathForDotNetTwo.
After changing the name and hitting Enter the new method creation is completed and you will notice that Coderush also took a best guess as to what needs to be passed into the new method, more on this later. To get back to the original method all that needs to be done is press the Esc key.
And there we can see our new method call we just created. Then I repeated the process for the Else statement and called it UpdateHintPathForDotNetOne.
Now we have quite a few parameters that are being passed in unnecessarily to the new methods so we clean those up and here is the final refactored method:
Private Function UpdateHintPath(ByVal projfile As String) As Boolean Dim xmldoc As XmlDocument Dim blDoSave As Boolean = False Dim blUpdateHintPathOk As Boolean = True File.SetAttributes(projfile, FileAttributes.Normal) xmldoc = New XmlDocument xmldoc.Load(projfile) If GetDotNetVersion(projfile) = DOTNETVERSION.TWO Then UpdateHintPathForDotNetTwo(projfile, xmldoc, blDoSave) Else 'DOTNET_VER = 1.0 UpdateHintPathForDotNetOne(projfile, xmldoc, blDoSave) End If '//check DOTNET_VER Try If blDoSave Then xmldoc.Save(projfile) End If Catch ex As Exception If dispXMLoutput Then Output("<message loc=""updatehintpath""><![CDATA[ERROR while updating HintPath ! Failed to save : " + projfile + "]]> </message>") Output("<message loc=""updatehintpath""><![CDATA[" + ex.Message + "]]> </message>") Else Output(vbCrLf + "Error while updating HintPath ! Failed to save : " + projfile + vbCrLf + ex.Message, ConsoleTools.ConsoleColor.red) End If WriteLog(vbCrLf + "Error while updating HintPath ! Failed to save : " + projfile + vbCrLf + ex.Message) blUpdateHintPathOk = False Throw ex Finally xmldoc = Nothing End Try File.SetAttributes(projfile, FileAttributes.ReadOnly) Return blUpdateHintPathOk End Function
Visual Studio Code Metrics is now reporting that the Maintainability Index of the new UpdateHintPath is at a respectable 50. Finally remember to run the unit tests to make sure they are still all green.
Comments
Post a Comment