current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
1

Following is the code I am using to find a separate count for alphabets and numeric characters in a given alphanumeric string:

   Public Sub alphaNumeric(ByVal input As String)
        'alphaNumeric("asd23fdg4556g67gh678zxc3xxx")
        'input.Count(Char.IsLetterOrDigit)
        Dim alphaCount As Integer = 0 '<-- initialize alphabet counter
        Dim numericCount As Integer = 0 '<-- initialize numeric counter
        For Each c As Char In input '<-- iterate through each character in the input
            If IsNumeric(c) = True Then numericCount += 1 '<--- check whether c is numeric? if then increment nunericCounter
            If Char.IsLetter(c) = True Then alphaCount += 1 '<--- check whether c is letter? if then increment alphaCount
        Next
        MsgBox("Number of alphabets : " & alphaCount) '<-- display the result
        MsgBox("Number of numerics : " & numericCount)
    End Sub

Everything works fine for me. Let me know how I can make this simpler.

share|improve this question
up vote 4 down vote accepted

In general your code looks good, here are a few smaller remarks.

Method name:

Capitalize the name of your method and make it more meaningful. Use CountAlphaNumeric or something similar.

Comments in code:

You can omit the comments in your code. It speaks for itself what the code is doing, certainly because you use clear names for your variables.

IsNumeric() - Char.IsDigit():

In the .NET framework, there's the Char.IsDigit method, use this one instead:

If Char.IsDigit(c) = True Then numericCount += 1

MsgBox() - MessageBox.Show():

Although MsgBox is valid, it also comes from the VB era. In the .NET framework there's the MessageBox.Show method, use that one instead:

MessageBox.Show("Number of alphabets : " & alphaCount)

String.Format():

To insert variables in a string, use the String.Format instead of just concatenating the values:

Dim result As String = String.Format("Number of alphabets : {0}", alphaCount)

"Expression = True":

You can leave out the = True part in your if conditions, since the methods return a boolean value.

This is what the code now looks like:

Public Sub CountAlphaNumeric(ByVal input As String)

    Dim alphaCount As Integer = 0
    Dim numericCount As Integer = 0

    For Each c As Char In input
        If Char.IsDigit(c) Then numericCount += 1
        If Char.IsLetter(c) Then alphaCount += 1
    Next

    MessageBox.Show(String.Format("Number of alphabets : {0}", alphaCount))
    MessageBox.Show(String.Format("Number of numerics : {0}", numericCount)

End Sub

Using LinQ:

Although not always the best option, you can achieve the same result using LinQ, using the Enumerable.Count method:

Dim alphaCount = input.Count(Function(c) Char.IsLetter(c))
Dim numericCount = input.Count(Function(c) Char.IsDigit(c))

Here's the complete code using LinQ:

Public Sub CountAlphaNumericUsingLinQ(ByVal input As String)

    Dim alphaCount = input.Count(Function(c) Char.IsLetter(c))
    Dim numericCount = input.Count(Function(c) Char.IsDigit(c))

    MessageBox.Show(String.Format("Number of alphabets : {0}", alphaCount))
    MessageBox.Show(String.Format("Number of numerics : {0}", numericCount))

End Sub
share|improve this answer
    
+1 for Perfect description. acceptable answer. thanks for the guidelines – Sujith Karivelil Jan 29 at 10:29
up vote 2 down vote

Comments should describe why something is done, what is done should be described by the code itself by using meaningful names for variables and methods.

Speaking of methods, what would alphaNumeric() mean to somebody else ? Nothing.

Instead of IsNumeric() you should nowadays use Char.IsDigit(). This also applies to MsgBox() -> MessageBox.Show().

The Char.IsLetter() and Char.IsDigit() (or IsNumeric()) methods will return a boolean stating the evaluation. You don't need to compare the result like If Char.IsLetter(c) = True then you can simplify this to If Char.IsLetter(c) then

If the current char is a number, you don't need to check if it is a letter. So you should better use a if..else if. 

If Char.IsNumber(c) = True Then
    numericCount += 1
ElseIf Char.IsLetter(c) Then
    alphaCount += 1
End If
share|improve this answer
    
+1 thanks for the guidelines, this is what am actually excepting. thanks a lot – Sujith Karivelil Jan 29 at 10:19

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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