8
\$\begingroup\$

This is my first script in Python, and I am looking for tips and constructive criticism about code style, whether there are more efficient ways of doing things, etc. (not as interested in tips about the method, more so the actual code).

Method:

  1. Choose random no. blocks (between 2 & 6)
  2. Decide whether to start vowel or consonant (random).
  3. Generate all blocks of vowels (random vowels, vowel number between 1 & 3)
  4. Generate all blocks of consonants (as above).
  5. Merge together to create 'name'.
## Alien Name Generator 1.0
## 19/7/15
## Build names using alternating blocks of all consonants or vowels

import math
import random
random.seed(a=None, version=2) 
print("Alien name generator:")

# Useful parameters: 
NUM_NAMES = 100 # number of names to generate
mx_name_blocks = 5 # max number of blocks that can be used  
vbl = 3 # max length of a single vowel block
cbl = 2 # max length of a single consonant block

filenametosave = "namelist5.txt"

## NOTE: Alphabet characters are 97 (a) - 122 (z)
# vowels are (96 +): 1,5,9,15,21
vowel_numbers = [97, 101, 105, 111, 117] 
# consonant numbers:
consonant_list = list(range(97,123)) # start with full alphabet
for count2 in vowel_numbers: # remove vowels from alphabet 
    consonant_list.remove(count2) # list of consonants

namelist = list() # store names from the main loop

### Main Loop (generates a single word each time):

for count in range (0,NUM_NAMES): 

    # Random number of blocks to use:
    n_vcs = random.randint(2,mx_name_blocks)

    # Randomly start vowel or consonant:
    vc_start = int(round(random.random(),0)) # v = 0, c = 1

    # Work out number of vowel or consonant blocks: 
    n_v = math.ceil(n_vcs/2) - vc_start*(n_vcs%2) # no. vowels
    n_c = n_vcs-n_v # no. consonants

    ### Generate vowel block list:

    vowel_block_list = list() # store all vowel blocks

    for count0 in range(1,1+n_v): # loop for each block

        vlength = random.randint(1,vbl) # random length of vowel block

        ### Generate single vowel blocks:

        v_block = list() # Store single vowel block

        for count1 in range(1,1+vlength): # loop for each individual vowel
            v1_val = chr(vowel_numbers[random.randint(0,4)]) # random vowel
            v_block.append(v1_val) # create list of vowels in single block


        string_block = ''.join(v_block) # join to make string

        vowel_block_list.append(string_block) # create list of all vowel blocks


    ### Generate consonant block list:

    const_block_list = list() # store all consonant blocks

    for count3 in range(1,1+n_c):
        clength = random.randint(1,cbl) # random length of block

        ### Generate single blocks:

        c_block = list() # store single block 

        for count4 in range(1,1+clength):
            c1_val = chr(consonant_list[random.randint(0,20)]) # random character
            c_block.append(c1_val)

        cstring_block = ''.join(c_block) 

        const_block_list.append(cstring_block) # list of consonant blocks


    ### Combine vowel and consonant blocks, alternating: 

    if n_v > n_c:
        mixed = []
        for i in range(len(vowel_block_list)):
             mixed.append(vowel_block_list[i])
             if i < len(const_block_list):
                mixed.append(const_block_list[i])
    else: 
        mixed = []
        for i in range(len(const_block_list)):
             mixed.append(const_block_list[i])
             if i < len(vowel_block_list):
                mixed.append(vowel_block_list[i])

    aName = ''.join(mixed).title()
    namelist.append(aName)

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

# WRITE TO FILE:
f = open(filenametosave,'w')
f.write("Alien name generator names: \n"+"\n".join(namelist))
f.close()

Example output:

Gque
Iiolyeeejuui
Hacbiieq
Bxau
Jyuu
\$\endgroup\$
5
\$\begingroup\$

All of your code is top-level, there are no functions and the only way to understand this programme is reading it all, also it is not tested so developing it was probably difficult.

Here is how I would go about it, with extensive comments.


"""
Block based alien name generator.
"""

This is a module level docstring, the first thing that gets displayed when the user types help(your_module) it usually describes concisely the purpose of the whole script.

import doctest
import itertools
import random
import string
import time

The Python Standard Library is extensive, many modules make devoloping programs easier.

MIN_BLOCK_LENGTH = 1
MAX_BLOCK_LENGTH = 2
MIN_BLOCKS_NUMBER = 2
MAX_BLOCKS_NUMBER = 3
VOWELS = "aeiou"
CONSONANTS = ''.join(i for i in string.ascii_lowercase if i not in  VOWELS)
TARGET_FILENAME = "nice_alien_names.txt"

These are module level constants, they either contain values that would be meaningless to change such as VOWELS or configuration options such as MAX_BLOCK_LENGTH that I modified as I like best short names. They are written SHOUTCASE and should not be changed (Python won't stop you from doing so though).

def random_string(length, pool):
    """
    >>> random.seed(0)
    >>> random_string(5, "abc")
    'bbabc'
    """
    return ''.join(random.choice(pool) for _ in range(length))

Now we get to the real meat. This is a function, an individual block of code with a name that performs one task and only that, return gives that value back into the programme. You may see the use of a generator expression, it can greatly shorten and simplify code. It is also tested, and the tests serve also as documentation.

def random_vowel_block():
    return random_string(random.randint(MIN_BLOCK_LENGTH, MAX_BLOCK_LENGTH), VOWELS)

def random_consonant_block():
    return random_string(random.randint(MIN_BLOCK_LENGTH, MAX_BLOCK_LENGTH), CONSONANTS)

Two small helper functions, to have function names near to the specification.

def alternate(func_1, func_2, times):
    """
    Returns a list containing `times` results
    of the two functions alternated.

    >>> list(alternate(lambda: 1, lambda: 2, 3))
    [1, 2, 1]
    """
    return list(itertools.chain(
        * zip( (func_1() for _ in range(times)),
               (func_2() for _ in range(times)))))[0:times]

This function is probably more complex than it should be ;P, anyhow docstring and test simplify understanding. As you can see I abstracted the concept of alternation into a higher order function (a function that takes functions as input) that I will use in the main alien_name function.

def alien_name():
    """
    Generates a random alien name by concatenating a random
    number (between MIN_BLOCKS_NUMBER and MAX_BLOCKS_NUMBER)
    of random all-vowel and all-consonant blocks, of which each one
    has a length between MIN_BLOCK_LENGTH and MAX_BLOCK_LENGTH.

    >>> random.seed(0)
    >>> alien_name()
    'aiqmiu'
    >>> alien_name()
    'wtaa'
    """
    name = ''.join(alternate(random_vowel_block, random_consonant_block,
                     random.randint(MIN_BLOCKS_NUMBER, MAX_BLOCKS_NUMBER)))
    return name if random.randint(0, 1) else ''.join(reversed(name))

The most important function, as you can see it is very small, as we have already defined all the building blocks needed. It reads almost as plain English. The last line implies that is a 50% chance of reversal to allow names starting with consonants.

def save_liked_ones():
    print("Enter nothing to ignore, `l` or `like` to save and `end` to terminate and save.\n")
    good_names = []
    while True:
        candidate_name = alien_name()
        choice = input("{}: ".format(candidate_name)).lower()
        if not choice:
            pass
        if choice.startswith('l'):
            good_names.append(candidate_name)
        if choice == 'end':
            with open(TARGET_FILENAME, 'a+') as f:
                f.write('\n'.join(good_names))
            return

I took the liberty of writing a user interface to avoid saving dull or uninteresting names, as you can see the logic is being performed all inside alien_name() this function only handles user interface.

if __name__ == "__main__":
    doctest.testmod()
    random.seed(time.time())
    save_liked_ones()

This runs the tests that you see in the docs and shows the user interface if this file is run as a script, importing it will only cause the functions to be defined. Please note that random.seed is needed as I seeded the Random Number Generator before for ease of testing.


And the full programme, in case you want to play and experiment with it:

"""
Block based alien name generator.
"""

import doctest
import itertools
import random
import string
import time

MIN_BLOCK_LENGTH = 1
MAX_BLOCK_LENGTH = 2
MIN_BLOCKS_NUMBER = 2
MAX_BLOCKS_NUMBER = 3
VOWELS = "aeiou"
CONSONANTS = ''.join(i for i in string.ascii_lowercase if i not in  VOWELS)
TARGET_FILENAME = "nice_alien_names.txt"

def random_string(length, pool):
    """
    >>> random.seed(0)
    >>> random_string(5, "abc")
    'bbabc'
    """
    return ''.join(random.choice(pool) for _ in range(length))


def random_vowel_block():
    return random_string(random.randint(MIN_BLOCK_LENGTH, MAX_BLOCK_LENGTH), VOWELS)


def random_consonant_block():
    return random_string(random.randint(MIN_BLOCK_LENGTH, MAX_BLOCK_LENGTH), CONSONANTS)


def alternate(func_1, func_2, times):
    """
    Returns a list containing `times` results
    of the two functions alternated.

    >>> list(alternate(lambda: 1, lambda: 2, 3))
    [1, 2, 1]
    """
    return list(itertools.chain(
        * zip( (func_1() for _ in range(times)),
               (func_2() for _ in range(times)))))[0:times]


def alien_name():
    """
    Generates a random alien name by concatenating a random
    number (between MIN_BLOCKS_NUMBER and MAX_BLOCKS_NUMBER)
    of random all-vowel and all-consonant blocks, of which each one
    has a length between MIN_BLOCK_LENGTH and MAX_BLOCK_LENGTH.

    >>> random.seed(0)
    >>> alien_name()
    'aiqmiu'
    >>> alien_name()
    'wtaa'
    """
    name = ''.join(alternate(random_vowel_block, random_consonant_block,
                     random.randint(MIN_BLOCKS_NUMBER, MAX_BLOCKS_NUMBER)))
    return name if random.randint(0, 1) else ''.join(reversed(name))


def save_liked_ones():
    print("Enter nothing to ignore, `l` or `like` to save and `end` to terminate and save.\n")
    good_names = []
    while True:
        candidate_name = alien_name()
        choice = input("{}: ".format(candidate_name)).lower()
        if not choice:
            pass
        if choice.startswith('l'):
            good_names.append(candidate_name)
        if choice == 'end':
            with open(TARGET_FILENAME, 'a+') as f:
                f.write('\n'.join(good_names))
            return


if __name__ == "__main__":
    doctest.testmod()
    random.seed(time.time())
    save_liked_ones()
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you very much for the extremely comprehensive response!! \$\endgroup\$ – kjldfg Jul 21 '15 at 20:20
  • \$\begingroup\$ @kjldfg You are welcome :) \$\endgroup\$ – Caridorc Jul 21 '15 at 20:25
3
\$\begingroup\$

Coding style

[...] I am looking for tips and constructive criticism about code style [...]

See PEP8 for the coding style guide of Python. You have several violations.

Duplicated logic

Avoid duplicated code blocks like this:

if n_v > n_c:
    mixed = []
    for i in range(len(vowel_block_list)):
         mixed.append(vowel_block_list[i])
         if i < len(const_block_list):
            mixed.append(const_block_list[i])
else: 
    mixed = []
    for i in range(len(const_block_list)):
         mixed.append(const_block_list[i])
         if i < len(vowel_block_list):
            mixed.append(vowel_block_list[i])

The body of the if and else branches should be in a function, taking vowel_block_list and const_block_list as parameters, in the appropriate order for your purpose. For example:

def create_mixed(first, second):
    mixed = []
    for i in range(len(first)):
         mixed.append(first[i])
         if i < len(second):
            mixed.append(second[i])

Instead of appending characters one by one, a more idiomatic solution is to use zip and to combine the two strings. zip will combine the strings until the shortest common length, so you will have to append the remaining part if any:

def create_mixed(first, second):
    return ''.join([a + b for a, b in zip(first, second)]) + first[len(second):]

Use with when working with files

Instead of this:

f = open(filenametosave,'w')
f.write("Alien name generator names: \n"+"\n".join(namelist))
f.close()

Write like this:

with open(filenametosave, 'w') as fh:
    fh.write("Alien name generator names: \n" + "\n".join(namelist))

Notice that there's no need to .close the file handle, the with construct takes care of it automatically.

Code execution in global scope

Avoid code execution in the global scope. As a good first refactoring step, move top-level code inside a main() function, and call it from within this if-guard:

if __name__ == '__main__':
    main()
\$\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.