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 Value._IsBetween(0, 100,,Exclusive) Then
If Value1._IsBetween(0,100) Then

See how much easier it is read and understand.

Spoiler

You could also easily implement addition function (that utilise these functions) to do range check. LowerLimit <= Value < UpperLimit
For example the lower and upper bound (.Count) of a collection.
By calling the IsBetween function with the correct exclusivities automatically included.

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

We could also refactor out the x+'s

x += If(some_Condition, 1, 2)

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

Class CoOrdinates
' Cos I've got an object that encapsulates the notion of a coordinate.
' I can also pre-define a commonly used one. The Origin (0,0,0)
Shared ReadOnly Origin As Origin = New CoOrdinates(0,0,0) '
Private _X As Double
Private _Y As Double
Private _Z As Double
Public Sub New(X As Double, Y As Double, Z As Double)
_X = X : _Y = Y : _Z = Z
End Class
Public ReadOnly Property X() As Double
Get
Return _X
End Get
End Property
Public ReadOnly Property Y() As Double
Get
Return _Y
End Get
End Property
Public ReadOnly Property Z() As Double
Get
Return _Z
End Get
End Property
Public Override Function ToString() As String
Return String.Format("(X:= {0} Y:= {1} Z:={2} )", Me.X, Me.Y, Me.Z)
End Function
End Class

By doing this different projects can reference them. You don't repeat them in your project (which actual make them specific to your project because of the implicit project namespace.
[spoiler]
Yes you could use the Global namespace Global.AdamSpeight2008.SecretProject.Interface
but it's better to have a single implementation.

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.

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.

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.

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?

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.

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

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).