7
\$\begingroup\$

I am new to coding and looking for a few pointers on how I can improve my first project.

At work, I often need to create Medicare Beneficiary Identifiers (MBI) when creating test patients with Medicare coverage, and have to look up the format every time. I thought creating an MBI generator would be a great first project. Doing a quick search, I found a similar project on this site (which is what lead me here), but the questioner wants to create 10,000 records, whereas I may only need one or two MBIs at a time (their question can be found here: Sequential MBI generator). Being new and not understanding all of the code, I was nervous to follow in their path and end up with a ton of records, so I actually followed an example of a random password generator and tweaked it to meet my needs. As the outcome is vastly different from the linked example and seems verbose, I wondered if anyone with more experience would be able to give me some pointers to get me off to improve my work.

#MBI is 11 characters in the following format 
# 1 - 2 - 3 - 4 - 5 - 6 - 7 - 8 - 9 - 10 - 11
# C - A - AN- N - A - AN- N - A - A - N  - N
# C = Numeric 1 - 9
# N = Numeric 0 - 9
# A = Alphabetic A...Z; Not S, L, O, I, B, Z
# AN = Either A or N

import random
letter = 'ACDEFGHJKMNPQRTUVWXY' # not = B, I, L, O, S, Z
digit = str('0123456789')
partdig = str('123456789')
dig_let = digit + letter

while 1:
     mbi_return = 1
     mbi_need = int(input("How many MBI numbers do you need to generate?: "))
      for x in range(0,mbi_need):
         mbi = ""
         for x in range(0, mbi_return):
            mbi_char0 = random.choice(partdig)
            mbi_char1 = random.choice(letter)
            mbi_char2 = random.choice(dig_let)
            mbi_char3 = random.choice(digit)
            mbi_char4 = random.choice(letter)
            mbi_char5 = random.choice(dig_let)
            mbi_char6 = random.choice(digit)
            mbi_char7 = random.choice(letter)
            mbi_char8 = random.choice(letter)
            mbi_char9 = random.choice(digit)
            mbi_char10 = random.choice(digit)
            mbi = (mbi_char0 + mbi_char1 + mbi_char2 + mbi_char3 + mbi_char4 + mbi_char5 + 
            mbi_char6 + mbi_char7 + mbi_char8 + mbi_char9 + mbi_char10) # I imagine this could be much cleaner
            print(mbi)

Thanks in advance!

\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

Instead of while 1: please just use while True. while 1: is a throwback from old versions of C that didn't have stdbool.h. while True: is much most explicit a about what your intent is.


At the top you have

letter = 'ACDEFGHJKMNPQRTUVWXY' # not = B, I, L, O, S, Z
digit = str('0123456789')
partdig = str('123456789')
dig_let = digit + letter

Python actually has built-ins for these:

from string import digits, ascii_uppercase

# letter is the set of ascii characters minus B, I, L. . .
letter = "".join(set(ascii_uppercase) - {'B', 'I', 'L', 'O', 'S', 'Z'})
# Just use digits instead of digit
partdig = digits[1:]  # Remove the first digit
dig_let = digits + letter

That saves you from needing to type out each of the letters to include.

Also note, even if string.digits didn't exist, you could have also defined digit as:

>>> digit = "".join(str(n) for n in range(0, 10))
>>> digit
'0123456789'

Also, all variable names here should be lowercase, separated by underscores. partdig should be part_dig, or part_digits, or even better: non_zero_digits.

I also think letter should be letters, since it's a collection of letters. It's a small change, but it lets your readers know that it's multiple letters, not just a single one.


Watch your indentation:

mbi_need = int(input("How many MBI numbers do you need to generate?: "))
 for x in range(0,mbi_need):

Especially in Python that matters a lot. If that was just a pasting error, it's a good idea to look over the code before posting just to double check that errors weren't introduced accidentally.

You're using a odd 5-space indentation in the loop though, which is part of the problem. Please use 4-space indentation.


for x in range(0,mbi_need):
    mbi = ""
    for x in range(0, mbi_return):

Both loops define a x variable! Since you never use x in either loop though, use _ instead:

for _ in range(0,mbi_need):
    mbi = ""
    for _ in range(0, mbi_return):

_ is a convention that says "I needed to create a name, but don't need the variable", which is the case here.


range(0, mbi_need)

0 is the implicit start; it's not necessary to specify it if you only otherwise need to specify the ending number. Just write:

range(mbi_need)

mbi_char0 = random.choice(partdig)
mbi_char1 = random.choice(letter)
mbi_char2 = random.choice(dig_let)
mbi_char3 = random.choice(digits)
. . .

Whenever you find yourself creating many similar variables, and you're differentiating them by putting numbers in the name, stop! You should likely be using a list instead. Because the make-up of the MBIs doesn't follow an easy pattern, fixing this isn't super straightforward, but it's still possible. First, I'd create a list holding the order of partdig, letter, dig_let, digits. . . which will define the order of the different character types:

mbi_pattern = [non_zero_digits, letters, digit_letters, digits,
               letters, digit_letters, digits, letters,
               letters, digits, digits]

This looks ugly, but it will clean up the code later. Sometimes all you can do is move the ugly bulk to the side. Once you've defined that list, creating a MBI is trivial and tiny:

mbi = "".join(random.choice(part) for part in mbi_pattern)
print(mbi)

Get each of the part sets, generate a random character from each of them, then join them into a string.


Your inner

for _ in range(mbi_return):

loop doesn't appear to be doing anything. It seems like it's doing a similar job as the other loop, except it will always be range(1), which will only run once, which means it isn't really a loop. I got rid of it because it isn't doing anything except complicating the code.

The same can be said about the while True as well. You want to generate 10000 codes, repeatedly, forever? The while True loop will never end since you never break from it. I also got rid of it because it is also complicating the code without good reason.


 mbi = ""

This isn't necessary. Even if you needed mbi in the outer scope, loops in Python don't create scopes like they do in other languages. mbi "defined" inside the loop can be accessed from outside of the loop.


In your remaining loop, you're creating an mbi, then just printing it. That doesn't allow you to do anything with the data though, like save it to file. It would be much cleaner to store the generated MBIs in a list so that they can potentially be used later. If you do that, your loop can be made into a list comprehension:

mbis = ["".join(random.choice(part) for part in mbi_pattern)
        for _ in range(mbi_need)]

Each MBI is generated using the same generator expression as before, but now it's wrapped in a list comprehension to generate multiple.


In the end, this is what I'm left with:

import random
from string import digits, ascii_uppercase

letters = "".join(set(ascii_uppercase) - {'B', 'I', 'L', 'O', 'S', 'Z'})
non_zero_digits = digits[1:]
digit_letters = digits + letters

mbi_pattern = [non_zero_digits, letters, digit_letters, digits,
               letters, digit_letters, digits, letters,
               letters, digits, digits]

mbi_need = int(input("How many MBI numbers do you need to generate?: "))

mbis = ["".join(random.choice(part) for part in mbi_pattern)
        for _ in range(mbi_need)]

print("\n".join(mbis))

And its usage:

How many MBI numbers do you need to generate?: >? 10
5V70VK4JP28
8Y12N77RC51
9JM2JN8RQ38
3X08DH7FH95
3MH6Y49KU87
6N70AC7MW75
9A67A62TU38
4A48C94QT38
2NP7TY0DC65
1GP8A57JQ27

As mentioned in the comment though, really, code should be tucked into functions. In larger programs, that eases testing and comprehension of your code.

Here, you could have a function that generates a single MBI, then use it to generate a list of them. I also always have a main function that ties the whole program together so I can control the execution of the code easier.

I also realized after I had my coffee that all the variables at the top are really constants, so they should be in UPPER_SNAKE_CASE.

After those changes, I'm left with:

import random
from string import digits, ascii_uppercase

LETTERS = "".join(set(ascii_uppercase) - {'B', 'I', 'L', 'O', 'S', 'Z'})
NON_ZERO_DIGITS = digits[1:]
DIGIT_LETTERS = digits + LETTERS

MBI_PATTERN = [NON_ZERO_DIGITS, LETTERS, DIGIT_LETTERS, digits,
               LETTERS, DIGIT_LETTERS, digits, LETTERS,
               LETTERS, digits, digits]


def generate_mbi():
    return "".join(random.choice(part) for part in MBI_PATTERN)


def main():
    mbi_need = int(input("How many MBI numbers do you need to generate?: "))
    mbis = [generate_mbi() for _ in range(mbi_need)]
    print("\n".join(mbis))
    
if __name__ == '__main__':
    main()
\$\endgroup\$
4
  • 1
    \$\begingroup\$ This is a very good review, indeed. One final improvement I would suggest either to the OP or to you, if you feel inclined to augment your answer: use functions. One to create a single MBI. The other to orchestrate the program: get user input and loop the needed number of times. \$\endgroup\$
    – FMc
    Aug 26, 2020 at 17:43
  • 1
    \$\begingroup\$ @FMc Yes. I completely agree with the use of functions here. I've started limiting that suggestion though to more "developed" programs where the benefits are much more obvious. I can certainly add something in though \$\endgroup\$ Aug 26, 2020 at 17:51
  • \$\begingroup\$ This is great! Thanks so much for the review! I knew there must be easier ways to address all of these issues (e.g., "...(ascii_uppercase) - {B, I,..etc.}), but I am limited by my cursory knowledge after a week of studying the language. This gives me a huge head start in learning these concepts. \$\endgroup\$
    – LostAsHeat
    Aug 26, 2020 at 20:38
  • 1
    \$\begingroup\$ @LostAsHeat Glad to help. I'm not sure if you're familiar with them, but the code here is using generator expressions, which are similar to list comprehensions. If you haven't used either before, they're very useful constructs. join is also extremely useful. \$\endgroup\$ Aug 26, 2020 at 20:48

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.