Critique/Rate My Code

  • (2 Pages)
  • +
  • 1
  • 2

26 Replies - 15951 Views - Last Post: 06 November 2012 - 07:24 AM

#1 depricated  Icon User is online

  • DLN-000

Reputation: 587
  • View blog
  • Posts: 2,101
  • Joined: 13-September 08

Critique/Rate My Code

Posted 24 October 2012 - 06:53 AM

So I'm a new programmer, or at least new to doing it professionally. I graduated a couple years ago and just landed my first job (yay me!) as a programmer. Naturally, I'm concerned with making sure my code is clean, and not redundant.

The language I'm using at the moment is VBA, for context. I'm hoping to occasionally post things like this, to learn best practice and give others something to see to help learn too. I hope that intent made sense?

Anyway, here's a function I built that I'm fairly happy with. I don't know that there's a built-in way to do this in VBA, but maybe there is a better way to do it all the same. This is RemoveDuplicates, which accepts a string (which should be a list of items delimited by a semicolon) and splits it, then compares the two and returns a string (with the items delimited by a semicolon) without duplicates.

I built this for an access DB that stores email addresses. We're pulling email addresses based on other criteria, and sometimes can pull the same person twice. This is intended to remove duplicates after retrieving addresses.

Private Function RemoveDuplicates(strDupes As String) As String

Dim astrSplitList() As String
Dim astrFilteredList() As String
Dim intBaseIterator As Integer, intInternalIterator As Integer
Dim boolFound As Boolean

astrSplitList = Split(strDupes, ";")
ReDim astrFilteredList(0 To 0)

For intBaseIterator = 0 To UBound(astrSplitList)
        
            For intInternalIterator = 0 To UBound(astrFilteredList)
                If astrSplitList(intBaseIterator) = astrFilteredList(intInternalIterator) Then boolFound = True
            Next
        
        If Not boolFound Then
            ReDim Preserve astrFilteredList(UBound(astrFilteredList) + 1)
            astrFilteredList(UBound(astrFilteredList)) = astrSplitList(intBaseIterator)
        End If
        
        boolFound = False

Next

RemoveDuplicates = Join(astrFilteredList, ";")

End Function


Didn't put this in the .NET forum initially cause I'm not concerned with the language as much as the code structure. . .

This post has been edited by depricated: 24 October 2012 - 08:56 AM


Is This A Good Question/Topic? 0
  • +

Replies To: Critique/Rate My Code

#2 cfoley  Icon User is offline

  • Cabbage
  • member icon

Reputation: 1940
  • View blog
  • Posts: 4,027
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 24 October 2012 - 06:04 PM

I'm going to clean it up the way I would do to any of my own code. I'm going to pretend there are unit tests for this and that none of the changes I make stop them from passing. I like my code to be minimal, clear and to read like English. So, with these goals in mind...

1. Functions that declare all there variables at the top add a lot of bulk for nothing. Some old versions of C used to make you do that. We're not using a language with such draconian rules. Let's start with that. This is what we are left with:

    Private Function RemoveDuplicates(ByVal strDupes As String) As String
        Dim astrSplitList() As String = Split(strDupes, ";")
        Dim astrFilteredList(0 To 0) As String

        For intBaseIterator = 0 To UBound(astrSplitList)
            Dim boolFound As Boolean = False

            For intInternalIterator = 0 To UBound(astrFilteredList)
                If astrSplitList(intBaseIterator) = astrFilteredList(intInternalIterator) Then boolFound = True
            Next

            If Not boolFound Then
                ReDim Preserve astrFilteredList(UBound(astrFilteredList) + 1)
                astrFilteredList(UBound(astrFilteredList)) = astrSplitList(intBaseIterator)
            End If

        Next

        RemoveDuplicates = Join(astrFilteredList, ";")
    End Function


Notice that moving the declaration of the boolean inside the loops meant I was able to remove the line that reset it at the end of the loop. This is one reason why proper scoping is important.

Your inner loop tests if an item is in the array. Going by the principle that each method should do one thing, we can extract that:

    Private Function RemoveDuplicates(ByVal strDupes As String) As String
        Dim astrSplitList() As String = Split(strDupes, ";")
        Dim astrFilteredList(0 To 0) As String

        For intBaseIterator = 0 To UBound(astrSplitList)
            Dim boolFound As Boolean = False

            boolFound = contains(astrSplitList(intBaseIterator), astrFilteredList)

            If Not boolFound Then
                ReDim Preserve astrFilteredList(UBound(astrFilteredList) + 1)
                astrFilteredList(UBound(astrFilteredList)) = astrSplitList(intBaseIterator)
            End If

        Next

        RemoveDuplicates = Join(astrFilteredList, ";")
    End Function

    Private Function contains(ByVal needle As String, ByVal astrFilteredList As String()) As Boolean
        For intInternalIterator = 0 To UBound(astrFilteredList)
            If needle = astrFilteredList(intInternalIterator) Then Return True
        Next
        Return False
    End Function



The next thing that is nagging at me is the variable names. It's bad practice to put the type in the variable name. That's another advantage of declaring the variable close to where you use it. You can see what type it is without having to encode it in the name. Single letter variable names are usually bad news... unless it's for a loop counter then it's about your best bet.

    Private Function RemoveDuplicates(ByVal values As String) As String
        Dim initialValues() As String = Split(values, ";")
        Dim filteredValues(0 To 0) As String

        For i = 0 To UBound(initialValues)
            Dim isFound As Boolean = contains(initialValues(i), filteredValues)

            If Not isFound Then
                ReDim Preserve filteredValues(UBound(filteredValues) + 1)
                filteredValues(UBound(filteredValues)) = initialValues(i)
            End If

        Next

        RemoveDuplicates = Join(filteredValues, ";")
    End Function

    Private Function contains(ByVal needle As String, ByVal haystack As String()) As Boolean
        For i = 0 To UBound(haystack)
            If needle = haystack(i) Then
                Return True
            End If
        Next
        Return False
    End Function



This is looking easier to read to me. My next issue is with changing the dimensions of your array every time you add one. When you do that, VB creates a new array and copies all the values into it. Instead, let's start with an array we know will be big enoung and trim it at the end:

Private Function RemoveDuplicates(ByVal values As String) As String
        Dim initialValues() As String = Split(values, ";")
        Dim filteredValues(0 To UBound(initialValues)) As String
        Dim uniqueCount As Integer = 0

        For i = 0 To UBound(initialValues)
            Dim isFound As Boolean = contains(initialValues(i), filteredValues)

            If Not isFound Then
                filteredValues(uniqueCount) = initialValues(i)
                uniqueCount += 1
            End If

        Next

        ReDim Preserve filteredValues(uniqueCount - 1)
        RemoveDuplicates = Join(filteredValues, ";")
    End Function

    Private Function contains(ByVal needle As String, ByVal haystack As String()) As Boolean
        For i = 0 To UBound(haystack)
            If needle = haystack(i) Then
                Return True
            End If
        Next
        Return False
    End Function


The next thing I'm going to be picky about is the function is doing two things. It's dealing with splitting/joining strings and with removing items from an array. Let's split the method in two:

    Private Function RemoveDuplicates(ByVal values As String) As String
        Dim allItems() As String = Split(values, ";")
        Dim filtered() As String = RemoveDuplicates(allItems)
        Return Join(filtered, ";")
    End Function

    Private Function RemoveDuplicates(ByVal values As String()) As String()
        Dim result(0 To UBound(values)) As String
        Dim uniqueCount As Integer = 0

        For i = 0 To UBound(values)
            Dim item As String = values(i)
            If Not contains(item, result) Then
                result(uniqueCount) = item
                uniqueCount += 1
            End If
        Next

        ReDim Preserve result(uniqueCount - 1)
        Return result
    End Function

    Private Function contains(ByVal needle As String, ByVal haystack As String()) As Boolean
        For i = 0 To UBound(haystack)
            If needle = haystack(i) Then
                Return True
            End If
        Next
        Return False
    End Function



At this point, I'm pretty happy. There are a few more things I might do. I'm using VB.NET so I'm not sure how they might apply to you:

For .. each loops
array.getUpperBound

    Private Function RemoveDuplicates(ByVal values As String) As String
        Dim allItems() As String = Split(values, ";")
        Dim filtered() As String = RemoveDuplicates(allItems)
        Return Join(filtered, ";")
    End Function

    Private Function RemoveDuplicates(ByVal values As String()) As String()
        Dim result(values.GetUpperBound(0)) As String
        Dim uniqueCount As Integer = 0

        For Each item In values
            If Not contains(item, result) Then
                result(uniqueCount) = item
                uniqueCount += 1
            End If
        Next

        ReDim Preserve result(uniqueCount - 1)
        Return result
    End Function

    Private Function contains(ByVal needle As String, ByVal haystack As String()) As Boolean
        For Each item In haystack
            If needle = item Then
                Return True
            End If
        Next
        Return False
    End Function



Finally, if I was doing this in VB.NET, I might come up with something like this, but that takes all the fun out of this exercise:

    Private Function RemoveDuplicates(ByVal values As String) As String
        Dim allItems() As String = Split(values, ";")
        Dim filtered() As String = allItems.Distinct()
        Return Join(filtered, ";")
    End Function


Was This Post Helpful? 4
  • +
  • -

#3 _HAWK_  Icon User is offline

  • Master(Of Foo)
  • member icon

Reputation: 1043
  • View blog
  • Posts: 4,057
  • Joined: 02-July 08

Re: Critique/Rate My Code

Posted 24 October 2012 - 06:39 PM

Nice cfoley I almost didn't see the last code block. I just made this example:
Dim str() As String = "1,2,3,2,4,5,5,6,7,2,5,3".Split(","c)
Dim temp() As String = str.Distinct.ToArray
MessageBox.Show(String.Join(", ", temp))


Private Function RemoveDuplicates(ByVal values As String) As String
    Dim allItems() As String = Split(values, ";")
    Dim filtered() As String = allItems.Distinct()
    Return Join(filtered, ";")
End Function


Option Strict On prefers(can't convert IEnumerable to String()):

Private Function RemoveDuplicates(ByVal values As String) As String
    Dim allItems() As String = Split(values, ";")
    Dim filtered() As String = allItems.Distinct.ToArray
    Return Join(filtered, ";")
End Function

This post has been edited by _HAWK_: 24 October 2012 - 06:40 PM

Was This Post Helpful? 3
  • +
  • -

#4 cfoley  Icon User is offline

  • Cabbage
  • member icon

Reputation: 1940
  • View blog
  • Posts: 4,027
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 25 October 2012 - 12:39 AM

I must have had option strict off. :)

This post has been edited by cfoley: 25 October 2012 - 05:21 AM

Was This Post Helpful? 0
  • +
  • -

#5 lucky3  Icon User is offline

  • Friend lucky3 As IHelpable
  • member icon

Reputation: 231
  • View blog
  • Posts: 765
  • Joined: 19-October 11

Re: Critique/Rate My Code

Posted 25 October 2012 - 01:23 AM

I see you're having fun with the code. I'm game:
Dim RemoveDuplicates As Func(Of String, String) = Function(x) Join((x.Split(";"c).Distinct.ToArray), ";")

Was This Post Helpful? 0
  • +
  • -

#6 cfoley  Icon User is offline

  • Cabbage
  • member icon

Reputation: 1940
  • View blog
  • Posts: 4,027
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 25 October 2012 - 01:56 AM

But then someone like me would come along and break that up into a couple more lines to make the intent clearer. ;)
Was This Post Helpful? 1
  • +
  • -

#7 depricated  Icon User is online

  • DLN-000

Reputation: 587
  • View blog
  • Posts: 2,101
  • Joined: 13-September 08

Re: Critique/Rate My Code

Posted 25 October 2012 - 05:21 AM

Wow, thank you! That's exactly the kind of feedback I was hoping for. I'm still new at this so it helps when I can take something I made and see how it can be done better. I appreciate that so much!

I had searched for a way to do this with built in functions but I didn't come across .Distinct. However, I'm using VBA(bleh) and not .NET, which seems to have some differences, so not sure if that's there or not. I'll give it a try here in a minute.

View Postlucky3, on 25 October 2012 - 02:23 AM, said:

I see you're having fun with the code. I'm game:
Dim RemoveDuplicates As Func(Of String, String) = Function(x) Join((x.Split(";"c).Distinct.ToArray), ";")

Would you mind breaking down what this is doing, for us noobs?
Was This Post Helpful? 0
  • +
  • -

#8 cfoley  Icon User is offline

  • Cabbage
  • member icon

Reputation: 1940
  • View blog
  • Posts: 4,027
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 25 October 2012 - 05:26 AM

No problem. A few books that have really helped me improve the quality of my code recently are Effective Java by Joshua Bloch, Refactoring by Martin Fowler and Clean Code by Robert C. Martin. They all use Java examples but most of the principles can be applied to other languages, especially .NET ones since the framework is so similar to the Java API.
Was This Post Helpful? 0
  • +
  • -

#9 depricated  Icon User is online

  • DLN-000

Reputation: 587
  • View blog
  • Posts: 2,101
  • Joined: 13-September 08

Re: Critique/Rate My Code

Posted 25 October 2012 - 05:35 AM

I think I'll start with Clean Code. I've seen that recommended a few times. Java is what I typically write in, too, so that will be a good frame of reference.
Was This Post Helpful? 0
  • +
  • -

#10 cfoley  Icon User is offline

  • Cabbage
  • member icon

Reputation: 1940
  • View blog
  • Posts: 4,027
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 25 October 2012 - 06:12 AM

It's doing what my code did:

Private Function RemoveDuplicates(ByVal values As String) As String
    Dim allItems() As String = Split(values, ";")
    Dim filtered() As String = allItems.Distinct()
    Return Join(filtered, ";")
End Function


Substitute filtered in the return statement:

Private Function RemoveDuplicates(ByVal values As String) As String
    Dim allItems() As String = Split(values, ";")
    Return Join(allItems.Distinct(), ";")
End Function


Substitute allItems:

Private Function RemoveDuplicates(ByVal values As String) As String
    Return Join(Split(values, ";").Distinct(), ";")
End Function


Now you've got essentially what lucky3 had. An important difference is that instead of writing a function, he wrote lambda, which is much like a function that you can pass as an argument to other functions.
Was This Post Helpful? 0
  • +
  • -

#11 lucky3  Icon User is offline

  • Friend lucky3 As IHelpable
  • member icon

Reputation: 231
  • View blog
  • Posts: 765
  • Joined: 19-October 11

Re: Critique/Rate My Code

Posted 25 October 2012 - 07:54 AM

Yeah, as cfoley and _HAWK_ did already. I just put everything together in lambda, as a joke I must say. cfoley and _HAWK_ shrunk your code to 3 lines, as an example how it can be done. I shrunk it into one line, as another example, the logic is the same.
Was This Post Helpful? 1
  • +
  • -

#12 depricated  Icon User is online

  • DLN-000

Reputation: 587
  • View blog
  • Posts: 2,101
  • Joined: 13-September 08

Re: Critique/Rate My Code

Posted 25 October 2012 - 08:14 AM

Hehe more pointedly I have no idea what a lambda is. /googling

You may have meant it as a joke, but its stuff like that that shows what I've still to learn :)
Was This Post Helpful? 1
  • +
  • -

#13 cfoley  Icon User is offline

  • Cabbage
  • member icon

Reputation: 1940
  • View blog
  • Posts: 4,027
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 25 October 2012 - 08:23 AM

When you have something like the .NET framework, you can accomplish a lot in one line of chained function calls. If you start messing with lambdas you can write entire programs in one line.

There is a trade off between clarity and brevity here!
Was This Post Helpful? 0
  • +
  • -

#14 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2241
  • View blog
  • Posts: 9,412
  • Joined: 29-May 08

Re: Critique/Rate My Code

Posted 25 October 2012 - 08:27 AM

And all of the above examples fail if the argument of the string parameter is null. Remember that a String is a reference type in .net.
Was This Post Helpful? 1
  • +
  • -

#15 cfoley  Icon User is offline

  • Cabbage
  • member icon

Reputation: 1940
  • View blog
  • Posts: 4,027
  • Joined: 11-December 07

Re: Critique/Rate My Code

Posted 25 October 2012 - 08:42 AM

That's an important point but sometimes failing really is the best option. A null value being passed to a private function is almost certainly due to a bug. (If it's not due to a bug then It's due to some very sloppy design.) In that case, you want the program to fail as soon as possible so the stack trace points you to the source of the error.
Was This Post Helpful? 0
  • +
  • -

  • (2 Pages)
  • +
  • 1
  • 2