6
\$\begingroup\$

Aim: To have only one of each character on the Return string:

Public Shared Function CheckForDuplicates(ByVal vCharCheck As String) As String

    Dim vDeDuplicated As String = ""
    Dim i As Integer

    For i = 1 To vCharCheck.Length ' Count length of string

        'vDeDuplicate is blank to start so will always capture first character
        'vDeDuplicate is then checked against each character in the incoming string to see if it contains this character already
        'thus deduplicting the string

        If vDeDuplicated.Contains(Mid(vCharCheck, i, 1)) Then   
            'Check the new string to see if it exists, if it does it is not allowed 
        Else
            'The character is not found so we add it
            vDeDuplicated += Mid(vCharCheck, i, 1)
        End If

    Next

    Return vDeDuplicated
End Function
\$\endgroup\$
  • 1
    \$\begingroup\$ I think we are missing some code, some of this doesn't make sense, how is vCharCheck determined/populated? where do you assign a value to vDeDuplicated? \$\endgroup\$ – Malachi Feb 7 '14 at 17:41
  • \$\begingroup\$ Added a note. A string input to a function \$\endgroup\$ – indofraiser Feb 7 '14 at 21:01
  • \$\begingroup\$ @indofraiser It would be easier if you added the function definition to the code. \$\endgroup\$ – konijn Feb 7 '14 at 21:17
7
\$\begingroup\$

Of course you can eliminate the loop by using the Distinct method of the string, which won't care if the string is empty or not. Assuming vCharCheck is the input string, it would look something like this:

Return New String(vCharCheck.Distinct.ToArray)
\$\endgroup\$
  • 2
    \$\begingroup\$ In a similar vein: String.Concat(vCharCheck.Distinct) \$\endgroup\$ – recursive Feb 10 '14 at 21:20
5
\$\begingroup\$

you should change the code around a little bit so that if the string is empty it will exit the code before hitting the loop. and you should use a foreach instead of a for and get rid of all the Mid stuff

For Each Character As Char In vCharCheck
    If Not vDeDuplicated.Contains(Character) Then
        vDeDuplicated += Character
    End If
Next

the loop won't run if the Length of vCharCheck is less than 1 so you don't really have to check that it contains a first letter.

I am sorry it will run, infinitely I think, because it will start at 1 and loop until it reaches 0 and nothing will happen inside the loop

\$\endgroup\$
2
\$\begingroup\$

There are also two minor improvements that could increase the performance of your function - although they do add a little bit of complexity to the final code.

  1. Decrease complexity using a structure that allows constant time character lookup.

    Your implementation uses the result string (vDeDuplicated) as a placeholder for the unique characters. Therefore, for every character c read from the input string, there is a call to vDeDuplicated.Contains(c) - which executes a linear search on vDeDuplicated. That could be avoided if we used a hashtable or an array, indexed by the character code itself. We can now check for repeated characters in constant time:

    'if the string is encoded in ASCII, we only need 128 elements 
    'to store every existing character. 
    'For unicode strings, you can change it to 65536
    Dim vDeDuplicated(128) As Boolean 
    Dim result As String = ""
    
    For Each Character As Char In vCharCheck
        'using the character numeric code as an index - constant time lookup
        If Not vDeDuplicated(AscW(Character)) Then
            result += Character
            vDeDuplicated(AscW(Character)) = True
        End If
    Next
    
    Return result
    
  2. Use a StringBuilder to generate your result.

    Since strings are immutable in VB.NET, using the string concatenation operator (+) will always generate new strings, leading to a significant performance penalty. As a rule of thumb, you should always try to use StringBuilder when the number of concatenations you need to perform is not known at compile time. These are some nice articles on that matter:

\$\endgroup\$
  • 1
    \$\begingroup\$ I agree with your second point that we should be using a StriungBuilder, but the first point looks like we are adding complexity that isn't needed here. the other two answers cover this situation much better than adding a new variable array into the mix, it just doesn't seem logical to me. \$\endgroup\$ – Malachi Feb 11 '14 at 16:52
  • 1
    \$\begingroup\$ Yes, I agree! At least from the point of view of code quality/maintainability - and that's why I added the remark that "they do add a little bit of complexity to the final code.". However, one cannot ignore the fact that we are decreasing complexity from O(n^2) to O(n) by adding a couple of lines (this is actually a pretty well known technique for implementing character lookup efficiently). The OP was not concerned with performance/complexity, but nonetheless I think this is an aspect that could/should be addressed in code review. Accepted answer is definitely much simpler, though. \$\endgroup\$ – rla4 Feb 11 '14 at 17:18
  • \$\begingroup\$ I will give it the UpVote, I was on the fence and you pushed me off of it onto the upvote side \$\endgroup\$ – Malachi Feb 11 '14 at 17:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.