Page 1 of 1

Refactoring Rate Topic: ***** 1 Votes

#1 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2256
  • View blog
  • Posts: 9,444
  • Joined: 29-May 08

Posted 01 December 2013 - 01:16 AM

*
POPULAR

Refactoring

OK you've gotten your code to compile and it produces the correct and desire output.
Now it's time refactor that spaghetti you call code.

Why Refactor?

Refactoring
  • Cleaner looking Code
  • Easier to reason about
  • Easier to maintain
  • Easier to alter
  • Easier to debug
  • Reusability.
  • Potentially Smaller size
  • Potentially Quicker code.





Let's start with the easiest.

Are the identifiers meaningful and informative?
  • Namespaces
  • Class Names
  • Structure Names
  • Interfaces.
  • Method Names
  • Field Names
  • Parameter Names
  • Variable Names

Nope,then rename them.




One Class Or Structure per a file.
You can also place then in VS Solution folders, that reflect the Namespaces

What if your it has multiple inner classes? Use Partial Classes.
Partial Class
End Class



This approach makes it a lot easier for different people to work on different parts of the design, where the version control system restricts access to a single user per a file.





Don't repeat yourself.

Are you repeating sections of code, then it a good indication you may need a function, or to store the result of the function. This is all about looking for commonalities in your code base and minimising them.

For example doing bounds check to see if a value is between to limits.

If (0 < Value) AndAlso (Value <= 100) Then

If (0 < Value1) AndAlso (Value < 100) Then



Can be refactored into a function (snippet: _IsBetween)
If Value._IsBetween(0, 100,,Exclusive) Then
If Value1._IsBetween(0,100) Then


See how much easier it is read and understand.
Spoiler


For example conditionals.
Dim result = (x < 100) AndAlso (x> 0) OrElse (y = 10)
If result Then



Don't rely operator precedence.
Make it explicit by using parenthesis ( )

Take the previous code example
Do you mean?
Dim result = ( (x < 100) AndAlso (x> 0) ) OrElse (y = 10)


or
Dim result = (x < 100) AndAlso ( (x> 0) OrElse (y = 10) )



Stop comparing against a Boolean in an IF-Statement, when the result is already a Boolean.
If result = True Then
Else
End If


Also remove any empty clauses.
Just using the Else Clause then invert the condition by prefix it with a Not
If x = 10 Then
Else
 ' Only do this
End If


Can be rewritten as
If Not (x = 10) Then
End If


or use the complementing operator
If x <> 10 Then
End If


List of complementing operator
Op     /Op
  =     <>
 <>      =
  <     >=
 <=      <
  >     <=
 >=      <



If the clause contains a single statement then use the single line if statement.
If {boolean} Then {do this}

If the both clauses manipulate the same variable, then it could be combined into a single IF-Expression.
Dim x = 2
IF some_Condition Then
   x = x + 1
Else 
   x = x + 2
End If


Refactors into
Dim x = 2
x = If(some_Condition, x+1, x+2)


Notice the repetition we could also eliminate?
Spoiler


Simplify the conditions.
Refer back to your Boolean Algebra and De Morgan's Laws.




Don't be afraid to make additional Classes, Structure or Interfaces
You can then exploit the compiler and the type-checking it does on your behalf.

Using variables that in association represent something or concept then make a new class / structure to encapsulate and capture that notation. For x , y , z are representing 3d coordinates then define a new coordinates class.
Always seem to be passing around the same variables to methods. Then make a new class / structure with those variable. Create and instance with those values and pass that around instead.
Now you can refer to them as a single object.
For example co-ordinates
' Co-Ordinates 1
Dim x As Double = -1
Dim y As Double = -2
Dim z As Double = -3
' Co-Ordinates 2
Dim x1,y1,z1 As Double


Spoiler





Use properties with backing non-public backing field.
This means no public fields at all, which allows the object to control it's own internal state via properties, methods and functions.



Try and make your objects as Immutable as possible.
This mean only have ReadOnly Properties and non-public fields, and the any change of (internal) state must result in new instance that reflects that change. This current instance can not change any of it's state.




Exploit Generics

This will allow you write one method or implementation that is applicable to many similar implementations.
For a example this will swap the contents of two variables, without boxing then unboxing.
<Extension>
Public Sub SwapWith(Of T)(ByRef x As T, ByRef y As T)
  Dim tmp As T = x
  x = y
  y = tmp






Utilise and Exploit Inheritance

Let's say you want to implement some for of unit system.

Start with a non generic base class

Public MustInherit Class UnitBase
   Protected Friend _Symbol As String

   Friend Sub New(Symbol As String)
     _Symbol = Symbol
   End Sub

   Public ReadOnly Property Symbol As String
       Get
         Return _Symbol
       End Get
  End Property
  Public Overrides Function ToString() As String
     Return _Symbol
  End Function
End Class


Then a more specific UnitValue class

Public MustInherit Class UnitValue
 Inherits UnitBase
  Protected Friend _Value As Double
  Friend Sub New(Value as Double, Symbol As string)
    MyBase.New(Symbol)
    _Value=Value 
  End Sub 
  Public ReadOnly Property Value As Double
     Get
       Return _Value
    End Get
  End Property

  Public Overrides  Function ToString() As String
     Return String.Format("{0}{1}", Me.Value, MyBase.ToString())
  End Function
End Class



Now a more generic UnitValue class. This allows use to exploit restricted generics at the operator level
Public MustInherit Class UnitValue(Of T As {New, UnitValue(of T)})
  Inherits UnitValue 
  Implements IComparable(of T)

   Friend Sub New(Value As Double, Symbol As String)
     MyBase.New(Value,Symbol)
   End Sub

Public Shared Operator +(ByVal lhs As UnitValue(Of  T), ByVal rhs As UnitValue(of  T)) As T 
      Return  New T With { ._Value =  lhs.Value + rhs.Value}
  End Operator
  Public Shared Operator -(ByVal lhs As UnitValue(Of  T), ByVal rhs As UnitValue(of  T)) As T 
      Return  New T With { ._Value =  lhs.Value + rhs.Value}
  End Operator

  Public Function CompareTo(other As T) As Integer Implements IComparable(Of T).CompareTo
    If Me.GetType() <> GetType(T) Then Return 1
    Return Me.Value.CompareTo(other.Value)
  End Function

  Public Shared Operator =( lhs As UnitValue(Of  T), rhs As  UnitValue(Of  T)) As Boolean
    Return lhs.Value=rhs.Value 
  End Operator
  
  Public Shared Operator <>( lhs As  UnitValue(Of  T), rhs As  UnitValue(Of  T)) As Boolean
    Return lhs.Value=rhs.Value 
  End Operator
  
  Public Shared Operator >=( lhs As  UnitValue(Of  T), rhs As  UnitValue(Of  T)) As Boolean
    Return lhs.Value>=rhs.Value 
  End Operator
  
  Public Shared Operator >( lhs As  UnitValue(Of  T), rhs As  UnitValue(Of  T)) As Boolean
    Return lhs.Value>rhs.Value 
  End Operator
  
  Public Shared Operator <=( lhs As  UnitValue(Of  T), rhs As  UnitValue(Of  T)) As Boolean
    Return lhs.Value<=rhs.Value 
  End Operator
  
  Public Shared Operator <( lhs As  UnitValue(Of  T), rhs As  UnitValue(Of  T)) As Boolean
    Return lhs.Value<rhs.Value 
  End Operator
End Class



OK we've sorted out the base classes lets implement a few Units.
Metres
Public NotInheritable Class Metres
  Inherits UnitValue(of Metres)
  Public Sub New()
    Me.New(0.0)
  End Sub
    Public Sub New( Value As Double)
     MyBase.New(Value,"m")
  End Sub

End Class


Feet
Public NotInheritable Class Feet
  Inherits UnitValue(of Feet)
  Public Sub New()
    Me.New(0.0)
  End Sub

  Public Sub New( Value As Double)
     MyBase.New(Value,"'")
  End Sub

End Class



By using inheritance we can place common functionality in a parent class, which because is inherited can be used in the child classes. For example we haven't had to replicate and duplicate the implementation of addition and subtraction for each UnitValue. If the implementation a Unit needs can be specific implementation it can added to that specific class's implementation or overloaded.




Separate the GUI/UI from the Logic and the Logic from the Data

By separating the design out into clear and distinct layers. Data -> Processing -> Control -> GUI
This is approach is often know by the Model View Controller approach.

By layering the design we can easily change the GUI (or implement multiple different GUI over the same Model layer).

Or change the Data Layer (for example use a different database) without affect the other layers.




Design Against an Interface rather than specific Classes / Structures

By doing this you don't tied your design to a specific implementation, just so long as the implementation has the same interface design you work with.
This approach is the secret behind how plugins work.




On to the hardest of them all.

Think about the design before starting to code.

Plan the design on whiteboard.





Do you have any other tips for others that you use when refactoring your code?
If so, add them as a reply to this thread.

Is This A Good Question/Topic? 5
  • +

Replies To: Refactoring

#2 andrewsw  Icon User is offline

  • Fire giant boob nipple gun!
  • member icon

Reputation: 3360
  • View blog
  • Posts: 11,397
  • Joined: 12-December 12

Posted 01 December 2013 - 04:47 AM

Nice summary ;)

I would also include: rename ALL controls from their defaults of the (meaningless) Button1, Button2, TextBox1.
Was This Post Helpful? 1
  • +
  • -

#3 code_m  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 24
  • View blog
  • Posts: 202
  • Joined: 21-April 09

Posted 03 December 2013 - 06:06 PM

View Postandrewsw, on 01 December 2013 - 06:47 AM, said:

Nice summary ;)

I would also include: rename ALL controls from their defaults of the (meaningless) Button1, Button2, TextBox1.


More importantly: Remove design only variables from your class. For example, that label that just tells the user what an input box is for and never changes, it does not need a reference!

(Change "Generate Member" to False in the properties, this will remove the variable from the partial class and place it locally with the InitializeComponent function.)


AdamSpeight2008, on 01 December 2013 - 03:16 AM, said:

If the clause contains a single statement then use the single line if statement.

If {boolean} Then {do this}


I very much disagree with this! In fact, unless you are fairly certain that a single line will remain a single line, you should break it out like any other statement. If you need to add a statement to the block it really is a hassle to modify that single line into five lines just because the previous author thought it might be easier to read. Adding a statement to a broken-out statement is a single enter keystroke, but adding a statement to a single line is four (or more!) keystrokes before you can even start.

I would much rather you take up more vertical space over horizontal space. Some authors (like myself) prefer to use a command line to read code and often end up limited to only 80 columns.




A similar preference of mine:

Do not wrap the entire body of a function within a flow control.

Rewrite:
Sub Button1_Click(sender As Object, args As EventArgs)
    If (sender = Me.button1)
        'Body of code
        ' ;
        ' ;
        ' ;
        ' ;
        'End of body
    End If
End Sub


To the following:
Sub Button1_Click(sender As Object, args As EventArgs)
    If (sender <> Me.button1)
        Return
    End If

    'Body of code
    ' ;
    ' ;
    ' ;
    ' ;
    'End of body
End Sub


Likewise re-write loops as re-cursive functions when the body of the loop is the entirety of the function.

This post has been edited by code_m: 03 December 2013 - 07:32 PM

Was This Post Helpful? 2
  • +
  • -

#4 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2256
  • View blog
  • Posts: 9,444
  • Joined: 29-May 08

Posted 03 December 2013 - 06:52 PM

View Postcode_m, on 04 December 2013 - 02:06 AM, said:

Likewise re-write loops as re-cursive functions when the body of the loop is the entirety of the function.


Recursive functions do have a penalty that it use up the stack (as tail-recursion optimisation isn't utilised fully exploited). Loops are highly optimised in .net. So I would tend to prefer the "traditional" loop.
But I'm curious to see examples where this isn't the case. Could you provide a code examples?
Was This Post Helpful? 0
  • +
  • -

#5 code_m  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 24
  • View blog
  • Posts: 202
  • Joined: 21-April 09

Posted 03 December 2013 - 07:26 PM

View PostAdamSpeight2008, on 03 December 2013 - 08:52 PM, said:

Recursive functions do have a penalty that it use up the stack (as tail-recursion optimisation isn't utilised fully exploited). Loops are highly optimised in .net. So I would tend to prefer the "traditional" loop.


I'm afraid I cannot provide solid example, however if the block of the loop is really that intensive, maybe you should consider another flow control altogether. For example, for an intense networking block consider using asynchronous operations to move it to another thread. I would still say readability of a recursive function outweighs the performance loss in most situations. Definitely a judgement call, but that's what makes refactoring really difficult.
Was This Post Helpful? 0
  • +
  • -

#6 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2256
  • View blog
  • Posts: 9,444
  • Joined: 29-May 08

Posted 03 December 2013 - 07:50 PM

Sometime the recursive version is slower.
Factorial
Recursive version is (O(2^n))
Factorial(n)
 If n < 2 Return n
 Return Fact(n-1) + Fact(n-2) ' Tail Recursion Optimisation can't happen cos of the addition
End Function


Loop based. (O(n))
Iterator Function Fact() As IEnumerable(Of Integer)
  Dim q As new Queue(of integer)({0,1})
  While True
    q.Enque( q.Sum())
    Yield q.Deque()
  End While
End Function

Fact(n)


Was This Post Helpful? 0
  • +
  • -

#7 CodingSup3rnatur@l-360  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 991
  • View blog
  • Posts: 971
  • Joined: 30-September 10

Posted 04 December 2013 - 04:41 PM

Good list, Adam.

Here are some more tips that I often use. Some are more like best practices and common mistakes (in my opinion) than types of refactoring, to be honest, but many are still relevant I think.

* Before doing any significant refactoring, ensure the code in question has a good set of unit tests that can be used to verify its behavior. This not only helps to detect any regressions that creep in as a result of the refactoring, but writing a good set of unit tests requires you to thoroughly understand the code, which will allow you to make better decisions whilst refactoring. Further, for code that doesn't currently have any unit tests, writing the unit tests for the code will usually make any areas that could benefit from refactoring painfully obvious.

* Always try to leave any code you work on in a better state than what it was in when you started working on it. Even if you only make a tiny improvement like narrowing the scope of a local variable, small changes will start to add up quickly, and the overall quality of the system will start to improve.

* If you see a comment explaining a piece of code, think about whether it would be possible to extract the commented code into a well named method that equivalently explains the meaning of the code, and then delete the comment. Assuming the meaning of the code remains clearly explained, self documenting code is almost always preferable over commented code.

* Delete any commented out code. Invest in some good source control software, and rely on that instead of cluttering the code with huge chunks of commented out code.

* Look with suspicion upon any class that queries the state of another object, and then conditionally performs processing based on that state. Should that processing be moved into the class that has the property, and the property then removed? Remember, object oriented principles tell us to place the behavior as close to the data it uses as possible.

* Where applicable (particularly where null is used to represent an undefined value (nullable value types excluded)), consider replacing null checks (which, again, clutter the code, are easy to miss out) with a Null Object.

* Remove excessive exception handling code. Exception handling code clutters and complicates the code, and can end up masking serious problems or leaving the program in an unknown state. Note that by excessive, I mean there is either an unnecessarily rigorous use (or, indeed, misuse) of try-catch statements, or the catch blocks catch an exception type that is too general for it to be able to guarantee that it can handle the exception successfully (the classic case being catching Exception without re-throwing it. I personally find I don't use all that many try-catch statements (relatively speaking) in my code.

* Add code to throw an exception when invalid input is received, particularly through public members. I generally find ArgumentNullException, InvalidOperationException and similar are probably amongst the most common types of exceptions I throw from within my code.

Combine this with my previous point, and you might say "but that'll crash your application". To that I say, absolutely. And that's exactly what I would want to happen. If I get one of those types of exception, it means I've seriously screwed up, so I want to know about it as soon as possible. If you don't want it to crash your program, test your application properly, and ensure you don't pass any member a value that it cannot handle. Failing fast in such situations is usually preferable.

An important, and probably often unrecognized corollary to this theory is that excessive use of automatic properties is a sign that you haven't understood the concepts of encapsulation and protecting a class' invariants.

* Use guard clauses to get the least common case(es) out of the way at the start of a method (thus making the normal flow through the method clearer), as described by code_m. I'm always using this technique.

This post has been edited by CodingSup3rnatur@l-360: 04 December 2013 - 05:05 PM

Was This Post Helpful? 1
  • +
  • -

#8 macosxnerd101  Icon User is online

  • Self-Trained Economist
  • member icon




Reputation: 10469
  • View blog
  • Posts: 38,802
  • Joined: 27-December 08

Posted 04 December 2013 - 05:48 PM

View PostAdamSpeight2008, on 03 December 2013 - 09:50 PM, said:

Sometime the recursive version is slower.
Factorial
Recursive version is (O(2^n))
<Snip>


Sorry to be pedantic, but you are talking about Fibonacci numbers here, not the factorial function. :)
Was This Post Helpful? 1
  • +
  • -

#9 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2256
  • View blog
  • Posts: 9,444
  • Joined: 29-May 08

Posted 07 December 2013 - 01:34 PM

View Postmacosxnerd101, on 05 December 2013 - 01:48 AM, said:

Sorry to be pedantic, but you are talking about Fibonacci numbers here, not the factorial function. :)/>


Which is also a good reason to refactor the method name, to make reflect its function. :whistling:
Was This Post Helpful? 0
  • +
  • -

#10 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2256
  • View blog
  • Posts: 9,444
  • Joined: 29-May 08

Posted 27 March 2014 - 01:47 AM

View Postcode_m, on 04 December 2013 - 02:06 AM, said:

A similar preference of mine:

Do not wrap the entire body of a function within a flow control.

Rewrite:
Sub Button1_Click(sender As Object, args As EventArgs)
    If (sender = Me.button1)
        'Body of code
        ' ;
        ' ;
        ' ;
        ' ;
        'End of body
    End If
End Sub


To the following:
Sub Button1_Click(sender As Object, args As EventArgs)
    If (sender <> Me.button1)
        Return
    End If

    'Body of code
    ' ;
    ' ;
    ' ;
    ' ;
    'End of body
End Sub


The second has a disadvantage, if you add logging and tracing to the code.
The multiple exit paths from the method would require addition method exiting functions, whereas the first only requires one set (at the beginning and at the end).
Was This Post Helpful? 0
  • +
  • -

Page 1 of 1