8
\$\begingroup\$

My code basically picks random letters and numbers from three lists and makes a random combination with them. I decided to write this code just for fun, however I feel like the code can be made shorter. I just don't know how.

So here is my code:

import random

caps=['A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','Q','R','S','T','U','V','W','X','Y','Z']
numbers=['1','2','3','4','5','6','7','8','9','0']
letters=['a','b','c','d','e','f','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z']



for j in range (4):
  lengthcaps=len(caps)
  indexcaps=random.randint(0, lengthcaps)
  fillcaps=caps[indexcaps-1]

  lengthnumbers=len(numbers)
  indexnumbers=random.randint(0, lengthnumbers)
  fillnumber=numbers[indexnumbers-1]

  lengthletters=len(letters)
  indexletters=random.randint(0, lengthletters)
  fillletter=letters[indexletters-1]
  
  list=[fillletter,fillcaps,fillnumber]
  index1list=random.randint(0,2)
  index2list=random.randint(0,2)
  index3list=random.randint(0,2)
  index4list=random.randint(0,2)
  index5list=random.randint(0,2)
  index6list=random.randint(0,2)
  
  fill1list=list[index1list]
  fill2list=list[index2list]
  fill3list=list[index3list]
  fill4list=list[index4list]
  fill5list=list[index5list]
  fill6list=list[index6list]
  
  print(fill1list + fill2list + fill3list + fill4list + fill5list + fill6list)

If this is duplicate, please feel free to tell me, because searching for things isn't one of my qualities.

\$\endgroup\$
9
  • 2
    \$\begingroup\$ Does this answer your question? Random string generating function \$\endgroup\$
    – alexyorke
    Apr 28 at 5:58
  • \$\begingroup\$ (I value clout in finding solutions/answers higher than knowing them. Developing / deriving original ones (if not necessarily new) is something to practice where finding pre-existing ones doesn't succeed or using them is no option.) \$\endgroup\$
    – greybeard
    Apr 28 at 8:38
  • 1
    \$\begingroup\$ @alexyorke Since we review code here to help the Original Poster to improve their code skills, duplicate questions don't have the same meaning they have on Stack Overflow. Duplicate questions are exact duplicates by the same author. \$\endgroup\$
    – pacmaninbw
    Apr 28 at 12:15
  • 2
    \$\begingroup\$ if someone wants to generate passwords with this or similar code: in this case you should use the secrets module instead of random \$\endgroup\$
    – Aemyl
    Apr 28 at 20:36
  • 1
    \$\begingroup\$ @Jéa the secrets module provides a cryptographically strong source of randomness, meaning that even if you know the last x output values of the random number generator, it is practically impossible to compute its internal state, so the next output is always unpredictable. That's not the case for the random module, which is the reason why it shouldn't be used for security purposes \$\endgroup\$
    – Aemyl
    Apr 28 at 20:59

5 Answers 5

22
\$\begingroup\$

Don't use caps, numbers and letters; those are all constants available from the string module.

Don't assign j since it isn't used; name the iteration variable _ instead.

Replace your length / index / slice with a random.choices.

Don't call a variable list, since 1. it shadows an existing type called list, and 2. it isn't very descriptive.

Rather than your manual, unrolled string appending, just use ''.join().

A strictly equivalent implementation could be

import random
from string import ascii_lowercase, ascii_uppercase, digits

for _ in range(4):
    fill_caps = random.choice(ascii_uppercase)
    fill_number = random.choice(digits)
    fill_letter = random.choice(ascii_lowercase)
    choices = (fill_letter, fill_caps, fill_number)
    word = ''.join(random.choices(choices, k=6))
    print(word)

but your algorithm has some odd properties that, according to your comments, you did not intend. The output word will have the choice of only one lower-case letter, one upper-case letter and one digit. The simpler and less surprising thing to do is generate a word from any of those characters:

import random
from string import ascii_lowercase, ascii_uppercase, digits

choices = ascii_lowercase + ascii_uppercase + digits

for _ in range(4):
    word = ''.join(random.choices(choices, k=6))
    print(word)
\$\endgroup\$
9
  • 1
    \$\begingroup\$ You could use ''.join(random.choices(choices, k=6)) toget rid of the list comprehension (and avoid a few slow attribute lookups). \$\endgroup\$
    – Graipher
    Apr 28 at 9:25
  • \$\begingroup\$ The second one was indeed what I was going for, do you have something where I can learn more about ' '.join()? Is there a specific reason why I should make _ instead of j? because I just used j because I picked a random letter to put there. \$\endgroup\$
    – Jéa
    Apr 28 at 9:42
  • 1
    \$\begingroup\$ Note that collecting all possible characters together and then choosing characters from the resulting list, choices = ascii_letters + ascii_uppercase + digits, has the consequence that each character in the resulting string only has probability 10 / (10 + 26 + 26) to be chosen. \$\endgroup\$
    – Stef
    Apr 28 at 13:57
  • 3
    \$\begingroup\$ @Jéa The convention is that _ should be used as if it "discards" its value. Essentially when you're calling a variable _, you're telling the reader "I won't use the value of this variable". \$\endgroup\$
    – Stef
    Apr 28 at 13:59
  • 1
    \$\begingroup\$ string.ascii_letters includes the upper case letters, so if you want to make your example really strictly equivalent, use string.ascii_lowercase as a substitution for letters \$\endgroup\$
    – Aemyl
    Apr 28 at 18:46
3
\$\begingroup\$

I feel like the code can be made shorter. I just don't know how.

If you want your code to be shorter, you could use a single list, like this.

import random

def rand_str(size):
    ascii_list = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
    res        = ""

    for i in range(size):
        res += ascii_list[random.randint(0, len(ascii_list) - 1)]

    return res

if __name__ == '__main__':
    # Test
    print(rand_str(16))
\$\endgroup\$
9
  • 4
    \$\begingroup\$ It's probably worth pointing out that "shorter" ≢ "better", in general. \$\endgroup\$ Apr 28 at 13:06
  • 1
    \$\begingroup\$ @TobySpeight "shorter" ≢ "better", true. But in this case I don't see the benefits of making rand_str longer or more complex. \$\endgroup\$
    – Leif
    Apr 28 at 13:37
  • 3
    \$\begingroup\$ Oh, agreed - though some of the other answers suggest using the named constants, i.e. ascii_list = string.ascii_lowercase + string.ascii_uppercase + string.digits which is clearer and less error-prone. I'm really challenging the assumption in the question rather than your answer! \$\endgroup\$ Apr 28 at 14:15
  • 1
    \$\begingroup\$ Yes, I wanted to keep the imports to a minimum but ascii_list = string.ascii_lowercase + string.ascii_uppercase + string.digits with import string would definitely make the code better. \$\endgroup\$
    – Leif
    Apr 28 at 14:25
  • 2
    \$\begingroup\$ Something that would be both shorter and better would be to use random.choice(ascii_list), or even better: eliminate the loop and use return ''.join(random.choices(ascii_list, k=size)) \$\endgroup\$
    – Jasmijn
    Apr 29 at 10:28
3
\$\begingroup\$

Disclaimer: I'm note 100% sure that your code does what you think it does. Anyway, various things can be improved in the code without changing the behavior.

Style

There is a Python style guide called PEP 8 which is definitely worth reading.

You'll find various tools to check if your code is compliant to it and even tools to reformat your code automatically (black).

Removing duplicated logic

The 3 first parts of the loop look highly similar. It could be a good idea to avoid having duplicated code which can be harder to read and maintain (see DRY).

In our case, we could implement a function performing this logic. Among the various other benefits: we would be able to give it a meaningful name, add some documentation, test it, etc.

For instance, we could have:

def get_random_element(lst):
    """Documentation is to be written here."""
    leng = len(lst)
    idx = random.randint(0, leng)
    return lst[idx -1]


for j in range(4):
    fillcaps = get_random_element(caps)
    fillnumber = get_random_element(numbers)
    fillletter = get_random_element(letters) 

Once we have this function, we could take this change to check that idx-1 is in the valid range with assert 0 <= idx - 1 < leng. This highlights an issue that I'll let you fix (the code somehow runs properly because negative indices are accepted by Python but this is probably not what you intended nor what another programmer would expect by reading quickly the code).

Once it is done, we could wonder whether everything performed with index1list and the other numbered variables could be rewritten using our get_random_element.

    lst = [fillletter, fillcaps, fillnumber]

    fill1list = get_random_element(lst)
    fill2list = get_random_element(lst)
    fill3list = get_random_element(lst)
    fill4list = get_random_element(lst)
    fill5list = get_random_element(lst)
    fill6list = get_random_element(lst)

(Note: I took this chance to rename list as lst so that we do not hide the list built-in function)

Finally, this function could be replaced by: random.choice() but for the sake of learning, it is probably best to try to write (and use) your own implementation.

Defining list of symbols

To get a list of letters, it would be somewhat shorter to write a string of letters and convert it to a list:

caps = list("abcdefghijklmnopqrstuvwxyz")
letters = list("abcdefghijklmnopqrstuvwxyz")
numbers = list("1234567890")

Even better, you can actually rely on values from the string module:

import string
letters = list(string.ascii_lowercase)
caps = list(string.ascii_uppercase)
numbers = list(string.digits)

Also, all the operations we use can work on strings as well so we do not need the conversion to list.

import string
letters = string.ascii_lowercase
caps = string.ascii_uppercase
numbers = string.digits

Then a valid question would be: do we still need these variables/constants at all ?

To be continued

\$\endgroup\$
2
\$\begingroup\$

Since strings are iterables, you can do this one liner:

import random

result = "".join(random.choice("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789") for i in range(0, 10))

Assigns a string containing 10 random characters from the provided string

In some cases, code editors might complain when you assign throwaway variables like i without accessing them. By convention, use _ instead to indicate that the variable is a throwaway.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ It may be worth being more explicit on the fact that the code is shorter but behaves very differently from the original code. (Though I am not able to tell if the original behavior is the one wanted, I can only assume that it is the case). \$\endgroup\$
    – SylvainD
    Apr 29 at 11:42
2
\$\begingroup\$

The current answers capture pretty much everything, but here's an alternative approach if you would be interested.

If you don't want to rely on an import for your ascii list, but still want to be sure you capture everything, you could construct it as follows:

ascii_list = [chr(i) for i in range(128) if chr(i).isalnum()]

This iterates over all ascii characters and picks the ones which are alphanumerical.

Then, you can construct your final list using the previously suggested method.

result = ''.join(random.choices(ascii_list, k=6))
\$\endgroup\$

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.