1
\$\begingroup\$

How can I simplify this code:

Public Class Form1
Private Sub BtnPositions_Click(sender As Object, e As EventArgs) Handles BtnPositions.Click
    ' 'txtSentence.Text = "The cat sat on the mat" ' Temp for testing

    If TxtAddSentence.Text = "" Then
        MsgBox("The text you are searching is empty, so there are no matches." & vbCrLf & "Please enter some text and retry.")
    End If

    Dim WordArray() = {""}
    Dim strSearchWord As String

    BuildWordArray(TxtAddSentence.Text, WordArray)
    strSearchWord = TxtAddWord.Text
    FindPositionOfWords(strSearchWord, WordArray)

    If TxtAddSentence.Text = "" Then
        MsgBox("The sentence box is empty, so there are no matches.")
    End If

End Sub
Private Sub BuildWordArray(ByVal strText, ByRef strArray)
    Dim strTextArray1() = Split(strText, " ")
    strArray = strTextArray1
End Sub

Private Sub FindPositionOfWords(ByVal strSearchWord, ByVal strWordList)

    Dim intIndex As Integer = 1
    Dim strPosn As String = ""
    For Each strWord As String In strWordList
        If UCase(strWord) = UCase(strSearchWord) Then
            If strPosn = "" Then
                strPosn = strPosn & Str(intIndex)
            Else
                strPosn = strPosn & "," & Str(intIndex)
            End If
        End If
        intIndex = intIndex + 1
    Next
    If strPosn = "" Then
        MsgBox("The Search Word Here Box Is Empty", MsgBoxStyle.Exclamation)
    Else
        MsgBox("The word '" & strSearchWord & "' was found at positions" & strPosn)
    End If
End Sub
End Class
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

please note I am not that great at vb.net

Variables - Define them! You haven't defined

   TxtAddWord
   TxtAddSentence (though I see txtSentence commented out)

are they defined on the form design?


Testing for something to be "" usually has a better way. I think something like this -

If String.IsNullOrEmpty(txtaddsentence) Then

Speaking of that if - you have it twice - why?

If TxtAddSentence.Text = "" Then
    MsgBox("The sentence box is empty, so there are no matches.")
End If

I can't see TxtAddSentence changing after the first check as there aren't any functions to return a string.


Speaking of strings, why are you always using

TxtAddSentence.Text

When you can just

    Dim TxtAddSentence = "The cat sat on the mat" ' Temp for testing
    Dim TxtAddWord = "sat"

Which are string type.


Naming - use variable names to describe what is happening, and avoid that hungarian notation.

Variable -> can be ->
TxtAddSentence      sentenceAddition
TxtAddWord          wordToAdd
strSearchWord       searchWord
strText             why? it's already sentenceAddition
strArray            why? it's already wordArray
strWordList         what is this? a List Type? An Array Type? A String?       
intIndex            index
strPosn             searchPosition
strWord             listedWord

Additionally, you're not giving a type to your variables:

ByVal strText, ByRef strArray)
ByVal strSearchWord, ByVal strWordList

Can be, as far as I can tell

Private Sub FindPositionOfWords(ByVal searchWord As String, ByVal wordArray() As String)
Private Sub BuildWordArray(ByVal sentenceAddition, ByRef wordArray)

It would be best practice to give a type to the variables in BuildWordArray - but I'm not entirely sure what type they should be.

Here -

Dim strPosn As String = ""

If this is position, why isn't it an integer? I would expect an integer, not a string.

\$\endgroup\$
1
\$\begingroup\$

There are a few things I would change. Some are performance related(though in your case they wont make any real difference, its good practice though) and others simply reduce the amount of code. Another change should make your code less likely to not find words. OK Here it is.


This line enforces proper programming practices in the case of type conversions and sever other things.

Option Strict On

This line enforces declaration of type rather than having the compiler infer them from other code. You'll get less problems in future programs if you always have this off.

Option Infer Off

This line imports code that allows you to use powerful(though not very readable) text handling tools. See later on.

Imports System.Text.RegularExpressions

Public Class Form1
    Private Sub BtnPositions_Click(sender As Object, e As EventArgs) Handles BtnPositions.Click

the next two lines have been moved to the top of the Class - it's often good practice to have your variables at the top of the definition so they are easier to find. Also, this is the bit that uses Regex as mentioned above in the imports at the top of the code. This line removes everything that is not a space or an alphabetic character. For example, if your sentence is "The cat is big, fat and hairy", if you try searching for "big", using your original code, you won't get a result because the word stored in strWordList would be "big," not "big"

        Dim WordArray() As String = {""}
        Dim sentenceToSearch As String = Regex.Replace(TxtAddSentence.Text, "[^a-zA-Z ]", "").ToLower

Check if both of your text boxes have text in. If they don't then there is no need to execute the rest of the code, hence the use of Exit Sub.

        If TxtAddSentence.Text = "" Then
            MsgBox("The sentence box is empty, so there are no matches.")
            Exit Sub
        ElseIf TxtAddWord.Text = "" Then
            MsgBox("The search box is empty, so there are no matches." & vbCrLf & "Please enter some text and retry.")
            Exit Sub
        End If

`BuildWordArray is better as a Function than as a Sub for readability and later on in larger programs, maintainability.

        WordArray = BuildWordArray(TxtAddSentence.Text)

In this situation however, the sub doesn't change any variables/objects outside of itself, so it is fine to leave it as it is.

        FindPositionOfWords(TxtAddWord.Text, WordArray)
    End Sub

This function only needs the one line as strTextArray1 only a local variable and you don't really need it in this case as you can directly return the results of the Split function. Also, this is the point where the string can safely be converted to lower case. Rather than have the program doing several conversions in the search loop, it is only done once here.

    Private Function BuildWordArray(ByVal strText As String) As String()
        Return Split(strText.ToLower, " ")
    End Function


    Private Sub FindPositionOfWords(ByVal strSearchWord As String, ByVal strWordList() As String)

See above also for my explanation of Regex

        strSearchWord = Regex.Replace(strSearchWord, "[^a-zA-Z ]", "").ToLower
        Dim strPosn As String = ""

Rather than use a For Each..Next loop and still use a variable as an index, just just a For..Next loop and use it's index. So here there is no definition of `intIndex1 until the loop here.

        For intIndex As Integer = 1 To strWordList.Count

See my explanation above about converting strings to lower case earlier on In the If statement you now only have the one instead of two If statements. This will again, in larger programs improve performance. In this code it's simpler just to keep adding the intIndex variable. The method Strcould be confusre with an array or some other collection, so it is better to use intIndex.ToString.There is no need in this case to have the extra check with regard to the adding of commas, Just trim off the last comma after the loop has completed.

            If strWordList(intIndex) = strSearchWord Then
                strPosn = strPosn & intIndex.ToString & ","
            End If
        Next
        strPosn.TrimEnd(","c)

The message box in you original version of the code showed an incorrect message. Earlier on in the new code, the search textbox has already been checked to make sure that it wasn't empty, so the Message should simply be that no results have been found. Telling the user that the box is empty would confuse them.

        If strPosn = "" Then
            MsgBox("No results found", MsgBoxStyle.Exclamation)
        Else
            MsgBox("The word '" & strSearchWord & "' was found at positions" & strPosn)
        End If
    End Sub
End Class

That's it. To make it easier for you to use the updated code .. here it is in one block

Option Strict On
Option Infer Off

Imports System.Text.RegularExpressions

Public Class Form1
    Private Sub BtnPositions_Click(sender As Object, e As EventArgs) Handles BtnPositions.Click
        Dim WordArray() As String = {""}
        Dim sentenceToSearch As String = Regex.Replace(TxtAddSentence.Text, "[^a-zA-Z ]", "").ToLower
        If TxtAddSentence.Text = "" Then
            MsgBox("The sentence box is empty, so there are no matches.")
            Exit Sub
        ElseIf TxtAddWord.Text = "" Then
            MsgBox("The search box is empty, so there are no matches." & vbCrLf & "Please enter some text and retry.")
            Exit Sub
        End If
        WordArray = BuildWordArray(TxtAddSentence.Text)
        FindPositionOfWords(TxtAddWord.Text, WordArray)
    End Sub

    Private Function BuildWordArray(ByVal strText As String) As String()
        Return Split(strText.ToLower, " ")
    End Function

    Private Sub FindPositionOfWords(ByVal strSearchWord As String, ByVal strWordList() As String)
        strSearchWord = Regex.Replace(strSearchWord, "[^a-zA-Z ]", "").ToLower
        Dim strPosn As String = ""
        For intIndex As Integer = 1 To strWordList.Count
            If strWordList(intIndex) = strSearchWord Then
                strPosn = strPosn & intIndex.ToString & ","
            End If
        Next
        strPosn.TrimEnd(","c)
        If strPosn = "" Then
            MsgBox("No results found", MsgBoxStyle.Exclamation)
        Else
            MsgBox("The word '" & strSearchWord & "' was found at positions" & strPosn)
        End If
    End Sub
End Class
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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