6
\$\begingroup\$

The following is my python code to find the number of iterations needed to generate random string iteratively to match the input string.

Here I have repeated a similar kind of for loop in both the functions getRandomString() and getMatchString(). I tried to avoid it but couldn't. I want this to be combined together into a single generateString() function with minimum iteration statements. Any suggestions to make it so or any other possible way to make the code better without complicating much?

The input is any string statements like "Hello, World! How Are Y@u doiNG?" and the output is the number of iterations needed to match the input string e.g 408. Your suggestions to improve the code will be helpful.

import random

goalString = input()
stringLen = len(goalString)

alphanum = '''abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ !@#&()–[{}]:;‘,'?/*1234567890'''
alphaLen = len(alphanum)


def getRandomString():
    randomString = ''
    for i in range(stringLen):
        randomString = randomString + alphanum[random.randrange(alphaLen)]
    
    return randomString 
            

def getMatchString():
    betterString = getRandomString()
    newString = ''
    iteration = 1 #included the iteration of generating randomString
    while betterString != goalString:
        charIndex = [i for i in range(stringLen) if betterString[i] == goalString[i]]
        for i in range(stringLen):
            if i in charIndex:
                newString = newString + betterString[i]
            else:
                newString = newString + alphanum[random.randrange(alphaLen)]
                    
        betterString = newString
        newString = ''
        iteration = iteration + 1
    
    return iteration

print(getMatchString())

Sample Input: "Hello, World! How Are Y@u doiNG?"

Sample Output: 408 ##May change depending on the random iteration##

\$\endgroup\$
4
  • 4
    \$\begingroup\$ Please do NOT edit the question after an answer has been posted, especially the code. Everyone who sees the post should see what the first person that posted an answer saw. Please read What should I do after someone answers?. \$\endgroup\$ – pacmaninbw Jan 26 at 13:26
  • \$\begingroup\$ You can ask follow up questions by linking to this question. \$\endgroup\$ – pacmaninbw Jan 26 at 13:30
  • \$\begingroup\$ @pacmaninbw Just thought to optimize the original code so as to get a more refined code. Won't do that from now and add up the link questions to the original one as suggested by you. \$\endgroup\$ – Jerin06 Jan 26 at 13:45
  • 6
    \$\begingroup\$ Please stop modifying the code, as you were asked by others. Protip: if you follow suggestion #1 on that list of options then you might be able to earn more reputation points. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 26 at 15:10
1
\$\begingroup\$

Since your result is only the number of iterations, you don't actually have to work with strings. Just keep track of how many characters are still wrong.

import random

goalString = input()

alphaLen = 82

def getMatchString():
    wrong = len(goalString)
    iteration = 0
    while wrong:
        wrong = sum(random.choices((0, 1), (1, alphaLen-1), k=wrong))
        iteration = iteration + 1    
    return iteration or 1

print(getMatchString())

Or handle the characters separately and take the max number of iterations:

import random

goalString = input()

alphaLen = 82

def getMatchChar():
    iteration = 1
    while random.randrange(alphaLen):
        iteration += 1
    return iteration

def getMatchString():
    return max(getMatchChar() for _ in goalString)

print(getMatchString())

A version that's perhaps slightly clearer:

import random

alphanum = '''abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ !@#&()–[{}]:;‘,'?/*1234567890'''
            
def getMatchChar(goalChar):
    iteration = 1
    while random.choice(alphanum) != goalChar:
        iteration += 1
    return iteration

def getMatchString(goalString):
    return max(getMatchChar(goalChar) for goalChar in goalString)

goalString = input()
print(getMatchString(goalString))
\$\endgroup\$
5
  • \$\begingroup\$ Liked the way how you approached solving the problem. Both options provide the right solution. I understood the first one which seems to be directly solving it. However, I couldn't get your idea behind the second option. Could you explain it a little bit? The second one seems to produce a different range of iteration results and you have taken maximum out of it. \$\endgroup\$ – Jerin06 Jan 27 at 5:42
  • 1
    \$\begingroup\$ @Jerin06 In your way, you keep going until all characters match. That is, the slowest-matching character is what matters. The one that took the maximum time to match. That's what my second option does. It produces the same range of iteration results (in getMatchString, not in getMatchChar, of course). \$\endgroup\$ – superb rain Jan 27 at 11:26
  • \$\begingroup\$ @Jerin06 Added yet another version that's like the second one but perhaps a little clearer (without being much longer or slower). \$\endgroup\$ – superb rain Jan 27 at 14:11
  • \$\begingroup\$ Yes, this one is much easier for me to understand. You have simplified the code very well. The only thing I am not sure about here is how long the while loop runs here and whether that increases the time complexity compared to the previous codes. If there is no problem with time complexity, I prefer this last option added by you. \$\endgroup\$ – Jerin06 Jan 27 at 14:32
  • \$\begingroup\$ @Jerin06 Compared to yours, mine decrease the time complexity, as I don't keep looking over and over again at already matched characters. \$\endgroup\$ – superb rain Jan 27 at 16:31
9
\$\begingroup\$

Find my suggestions below.


goalString = input()

Let the user know what to do, input accepts a string as an argument:

goalString = input('Type a string:')

alphanum = '''abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ !@#&()–[{}]:;‘,'?/*1234567890'''

If you need all printable characters you can use string.printable:

import string

alphabet = string.printable
# alphabet contains: 0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~

Or choose a combination of other constants in the string module.


Generating a random string

def getRandomString():
    randomString = ''
    for i in range(stringLen):
        randomString = randomString + alphanum[random.randrange(alphaLen)]
    
    return randomString 
  • Avoid using globals passing the length as an argument
  • String concatenation can be done more efficiently with join
  • Use random.choices to avoid the for-loop
  • PEP 8: function names should be lowercase, with words separated by underscores. Variable names follow the same convention as function names.
def generate_random_string(n):
    alphabet = string.printable
    return ''.join(random.choices(alphabet, k=n))

The function to match the string creates many strings using concatenation. It can be done more efficiently by using a list of booleans (as @rperezsoto suggested).

def get_matching_string(s):
    n = len(s)
    matches = [False] * n
    i = 0
    while not all(matches):
        matches = [matches[i] or s[i] == c for i, c in enumerate(generate_random_string(n))]
        i += 1
    return i

The key part is to use the operator or that "remembers" positive matches.


I want this to be combined together into a single generateString() function with minimum iteration statements

I would suggest keeping two separate functions as in your first solution. The code will be easier to test and reuse. Additionally, you can extract the logic to request the input and print the result from the function getMatchString with a main function:

def main():
    s = input('Type a string:')
    print(get_matching_string(s))

if __name__ == "__main__":
    main()
\$\endgroup\$
1
  • 2
    \$\begingroup\$ string.printable and random.choices make things easier here. Appreciate your effort in explaining them in detail. Yes, instead of focussing on reducing functions, I should create reusable functions. Good, you mentioned many helpful tips like PEP 8 for creating function names. Thanks! \$\endgroup\$ – Jerin06 Jan 26 at 14:51
3
\$\begingroup\$

It took me a while to understand the code but I hope that I understood it properly. Lets say you have the string "abcd"

   goal string      "abcd"
   1st iteration    "ftcP" -> you save the 3rd position as found
   2nd iteration    "aXlL" -> At this pint both 1st and 3rd positions have been found 
   3rd iteration    "...." -> Still 3rd and 1st 
   4th iteration    "UbDd" -> all positions have matched at least once. 
   Stop and return 4 as the result

If the previous example is what you want to achieve below is my code suggestion

from random import choice

pool = '''abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ !@#&()–[{}]:;‘,'?/*1234567890'''
GetNewChar = lambda : choice(pool)

def GetRandomString(stringLen):
    characters = [GetNewChar() for i in range(stringLen)]
    return ''.join(characters)

def IterationsToMatchString(goal_string):
    IsGoalAchieved = [False for i in goal_string]
    iterations = 0
    while not all(IsGoalAchieved): 
        iterations += 1
        test_string = GetRandomString(len(goal_string))
        Matches = [g==t for g,t in zip(goal_string,test_string)]
        for i,t in enumerate(IsGoalAchieved):
            if t is False: 
                IsGoalAchieved[i] = Matches[i]
    return iterations

example = input()
print(IterationsToMatchString(example))

regardless of which variable naming convention you use I suggest not using the same name for the function and the output unless you are writing a decorator function. In your "randomString" function you return a variable named "randomString" which makes the code difficult to understand and is likely to bug. I would suggest changing the name of the function to improve reading.

Also you abuse the global keyword. You can just pass arguments to the function. In this case your function randomString generates a random string of a fixed length. Therefore you can always call that function with the length as a parameter.

Also as you have seen I have substituted the usage of randrange by choice (but does the same). I prefer it because we are picking one item out of a certain sequence.I have used a lambda function to provide a more "readable" name that gives information about what is being done.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks for your suggestions. I will make the changes as mentioned by you to improve the code readability. I'll also go through your code and come back if I need any clarification. \$\endgroup\$ – Jerin06 Jan 26 at 10:36
  • \$\begingroup\$ Does your code work? I couldn't get output from your code due to some error message. \$\endgroup\$ – Jerin06 Jan 26 at 11:16
  • \$\begingroup\$ I forgot to "initialize" the iterations variable with "0" and also put an non neede argument in the GetNewChar. I have edited the post and it should work now \$\endgroup\$ – rperezsoto Jan 26 at 13:10
  • \$\begingroup\$ Yes, it is working now. You have used some wonderful techniques. Very helpful for me to enhance my coding. Thanks! \$\endgroup\$ – Jerin06 Jan 26 at 13:22
  • 1
    \$\begingroup\$ If you could please point me towards some reference for that I would love to learn more. I know that this code can get much better and I am aware that even within the answer the naming convention is not even consistent. And also feel free to edit the answer, I will be able to learn from the changes. \$\endgroup\$ – rperezsoto Jan 26 at 20:01

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.