Creating new string with only distinct letters

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
vDeDuplicated += Mid(vCharCheck, i, 1)
End If

Next

Return vDeDuplicated
End Function

• 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? – Malachi Feb 7 '14 at 17:41
• Added a note. A string input to a function – indofraiser Feb 7 '14 at 21:01
• @indofraiser It would be easier if you added the function definition to the code. – konijn Feb 7 '14 at 21:17

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)

• In a similar vein: String.Concat(vCharCheck.Distinct) – recursive Feb 10 '14 at 21:20

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

• – rolfl Feb 7 '14 at 17:47
• @rolfl, that makes sense. but shouldn't. VB – Malachi Feb 7 '14 at 17:50
• I think checking the string length before the foreach loop adds complexity. The foreach inherently checks for an empty string because if the string is empty, it will not execute. By removing the If, you have reduced nesting, made the code flow, and made it less confusing. – Jeff Vanzella Feb 7 '14 at 18:07
• @JeffVanzella the whole point of checking it outside is to keep the stuff inside of it from trying to run if it doesn't have to. – Malachi Feb 7 '14 at 18:11
• @JeffVanzella, that makes sense and I agree with you. plus I had the wrong variable in that if statement as well – Malachi Feb 7 '14 at 18:18

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:

• 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. – Malachi Feb 11 '14 at 16:52
• 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. – rla4 Feb 11 '14 at 17:18
• I will give it the UpVote, I was on the fence and you pushed me off of it onto the upvote side – Malachi Feb 11 '14 at 17:27