Take a look at this unwieldy mess:
1: Private Sub returnCodes()
2: code.Items.Clear()
3: If IsNothing(mycls.Dates) = False Then
4: If mycls.Dates.Rows.Count <> 0 Then
5: Dim myrow As DataRow
6: Dim myindex As Integer
7: For Each myrow In mycls.Dates.Rows
8: If getId(Input.Text) = Trim(myrow("id")) Then
9: For myindex = 0 To code.Items.Count - 1
10: If Trim(code.Items(myindex)) = Trim(myrow("year")) Then
11: Exit For
12: End If
13: Next
14: If code.Items.Count - 1 < myindex Then
15: code.Items.Add(myrow("year"))
16: Else
17: If String.Compare(Trim(code.Items(myindex)), Trim(myrow("year")), True) <> 0 Then
18: code.Items.Add(myrow("year"))
19: End If
20: End If
21: End If
22: Next
23: code.Sorted = True
24: If code.Items.Count > 0 Then setCode(code.Items(0))
25: End If
26: End If
27: End Sub
Pretty ugly huh? A couple of problems with this:
There are a high number of distinct paths through the code
The amount paths through a section of code is directly correlated to the chance of it containing a bug. You can't realistically test every single possible scenario on your code - its simply not possible, therefore the only answer is to reduce the possible paths your code can take.
Excessive looping
Loops use up resources - fact. A piece of code that loops 5 times is quicker than a piece that loops 500. It is therefore important to use looping carefully to efficiently use resources.
Its hard to debug
Chances are you won't be the only person who will have to read and understand your code. You might understand it fully when you write it, but what about a year down the line? What about your colleagues when you aren't about?
So that I could ultimately understand and subsequently modify, I had to simplify it whilst also keeping in mind the above points.
Now:
Not only is the code easy to understand, I actually increased performance dramatically. This process reduced loops from 2480 to 509
The most important lesson I've learnt is that there is simply no excuse for any good developer to submit code without removing the unnecessary crap. See this article for some of the hints on cleaning up your own code.
Excessive looping
Loops use up resources - fact. A piece of code that loops 5 times is quicker than a piece that loops 500. It is therefore important to use looping carefully to efficiently use resources.
Its hard to debug
Chances are you won't be the only person who will have to read and understand your code. You might understand it fully when you write it, but what about a year down the line? What about your colleagues when you aren't about?
Coding like this will only contribute to a slower, harder to debug, error prone software.
So that I could ultimately understand and subsequently modify, I had to simplify it whilst also keeping in mind the above points.
Now:
1: Private Sub returnCodes()
2: code.Items.Clear()
3:
4: If IsNothing(mycls.Dates) Then Exit Sub
5:
6: Dim myrow As DataRow
7:
8: For Each myrow In mycls.Dates.Rows
9: If getId(Input.Text) = Trim(myrow("id")) Then
10: If Not code.Items.Contains(myrow("year")) Then
11: code.Items.Add(myrow("year"))
12: End If
13: End If
14: Next
15:
16: code.Sorted = True
17:
18: If code.Items.Count > 0 Then setCode(code.Items(0))
19:
20: End Sub
Not only is the code easy to understand, I actually increased performance dramatically. This process reduced loops from 2480 to 509
The most important lesson I've learnt is that there is simply no excuse for any good developer to submit code without removing the unnecessary crap. See this article for some of the hints on cleaning up your own code.
I see this all too often at the web shop I work at. Generally speaking if you have more then 2 nested loops it's a good indication you need to break out something as you suggested. I wish more developers would grasp this concept. I'm enjoying reading your blog, keep up the good articles!
ReplyDeleteI believe quality software begins from the ground up - and that includes the quality of the code you write. Good code reduces the ways in which things can go wrong, which inevitably means reducing the actual amount of paths through the code. If statements that are as wide as they are long are definite code smells.
ReplyDeleteGlad you're enjoying the blog - I keep posting whatever my brain thinks!