2
\$\begingroup\$

I have written the below code which generates all possible permutations with repetitions of a string.

I want to make this program shorter and much faster, but I don't know how to do that.

Imports System.Text

Public Class Form1

    Private blkRpt As Integer
    Private perm As Double

    Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click

        Dim textlen As Double = TextBox1.Text.Length
        Dim rtb As Double = TextBox2.Text
        perm = textlen ^ rtb
        permutate()
    End Sub

    Private Sub permutate()

        For a As Integer = 0 To 1 Step 0
            Dim s As String = TextBox1.Text
            Dim len As Integer = TextBox2.Text
            Dim r As New Random
            Dim sb As New StringBuilder

            For i As Integer = 1 To len
                Dim idx As Integer = r.Next(0, s.Length)
                sb.Append(s.Substring(idx, 1))
            Next

            Dim sring As String = sb.ToString

            If RichTextBox1.Text.Contains(sb.ToString) Then
                blkRpt = blkRpt + 1
            Else
                RichTextBox1.AppendText(sring & vbNewLine)
           End If

           Dim s1 As String = TextBox1.Text
           Dim len1 As Integer = TextBox2.Text
           Dim r1 As New Random
           Dim sb1 As New StringBuilder

           For i As Integer = 1 To len1
               Dim idx1 As Integer = r1.Next(0, s1.Length)
               sb1.Append(s1.Substring(idx1, 1))
           Next

           Dim sring1 As String = sb1.ToString

           If RichTextBox1.Text.Contains(sb1.ToString) Then
                blkRpt = blkRpt + 1
           Else
                RichTextBox1.AppendText(sring1 & vbNewLine)
           End If

           Dim s2 As String = TextBox1.Text
           Dim len2 As Integer = TextBox2.Text
           Dim r2 As New Random
           Dim sb2 As New StringBuilder

           For i As Integer = 1 To len2
               Dim idx2 As Integer = r2.Next(0, s2.Length)
               sb2.Append(s2.Substring(idx2, 1))
           Next

           Dim sring2 As String = sb2.ToString

           If RichTextBox1.Text.Contains(sb2.ToString) Then
               blkRpt = blkRpt + 1
           Else
               RichTextBox1.AppendText(sring2 & vbNewLine)
           End If

           Dim s21 As String = TextBox1.Text
           Dim len21 As Integer = TextBox2.Text
           Dim r21 As New Random
           Dim sb21 As New StringBuilder

           For i As Integer = 1 To len1
               Dim idx21 As Integer = r21.Next(0, s2.Length)
               sb2.Append(s2.Substring(idx21, 1))
           Next

           Dim sring21 As String = sb21.ToString

           If RichTextBox1.Text.Contains(sb21.ToString) Then
               blkRpt = blkRpt + 1
           Else
               RichTextBox1.AppendText(sring21 & vbNewLine)
           End If

           If RichTextBox1.Lines.Length - 1 >= perm Then
               Exit For
           End If

           Label6.Text = blkRpt.ToString
           Label5.Text = RichTextBox1.Lines.Length - 1 & "/" & perm
       Next
    End Sub
End Class
\$\endgroup\$

migrated from stackoverflow.com Apr 17 '13 at 12:13

This question came from our site for professional and enthusiast programmers.

  • 1
    \$\begingroup\$ Please use Option Strict....implicit conversions (like Dim rtb As Double = TextBox2.Text) are dangerous. What is TextBox2.Text has "The first and the last" in it? \$\endgroup\$ – Tim Apr 14 '13 at 8:56
  • \$\begingroup\$ Have you run this code? Your outer for loop has Step 0 - which means the loop will never increment - you have an infinite loop. You're using random to get (I think) a random element of the string...but you could easily get the same element multiple times. Those are just two things that jump out. \$\endgroup\$ – Tim Apr 14 '13 at 9:05
  • 1
    \$\begingroup\$ Take a look at this SO question - Listing all permutations of a string/integer. Goes into a good deal of detail on how to do this. \$\endgroup\$ – Tim Apr 14 '13 at 9:06
  • \$\begingroup\$ I have ran this code, It will continue to loop until it generated all the possible combination \$\endgroup\$ – XtraCode Apr 14 '13 at 10:54
  • \$\begingroup\$ Every output in Richtextbox1 is unique. Look at richtextbox1.text.contains(sb.ToString) \$\endgroup\$ – XtraCode Apr 14 '13 at 10:55
5
\$\begingroup\$

I'm not going to rewrite this code right now, but I can highlight a few points:

  1. Naming.
    • Identifiers should have a meaningful name.
    • Disemvoweling is evil.
    • Adding a 1 after an identically-named variable is bad.
    • Adding a 2 after an identically-named variable is terrible.
    • Adding 21 instead of a 3 after an identically-named variable is careless.
    • If you think you need the same variable 3 times in one function, something's not right. Right? I think Tim's comment on your original post has a huge golden clue on that matter.
  2. UI is for presentation. Logic that isn't presentation logic shouldn't need to know there's a UI involved.
    • Decoupling your logic from the UI helps making your code testable.
    • Learn to pass parameters into your functions.
    • Knowing exactly what's involved is very hard to tell, we need to puzzle out the whole thing just to figure out what RichTextBox1 is supposed to contain; your naming isn't helping.
  3. Implicit string-to-numeric conversion is ugly and bug-prone, especially with user input.
    • Assume the user is a 3 year old that could click anywhere and enter anything in any TextBox.
  4. You have many, many, many things to worry about before you can think of improving performance.
    • Using Random isn't making it any faster. I don't think Random is warranted here.
    • Access the UI once to get the values you need, once to write the values you've computed; pass the values as parameters (I said that before, I'll say it again - here, done).
    • It's not the job of a permutation function to access the UI. Ever. Take values in (as parameters), do your thing, spit out a result (as a return value).
    • perm (bad name) shouldn't be an instance variable. Not even a parameter. It's only meaningful within the permutate function: that's where it belongs.

That's all folks! I might edit this post later with actual code.

\$\endgroup\$
  • 2
    \$\begingroup\$ +1 good points about the naming, UI, and testing. It does all tie into what I know about interfacing and good, testable coding. \$\endgroup\$ – Jamal Nov 16 '13 at 3:41

Your Answer

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