3
\$\begingroup\$

I'd like feedback on my revised code after my previous question.

The goal is to create a module which can display, update, save dictionaries as .csv files. I'll use this in another program to update its settings.

I'm learning Python so I'm looking for any and all critiques on mechanics and style.

import csv

# Imports a file as a dictionary with csv
def csv_import(filename):
    dictionary = {}
    for key, value in csv.reader(open(filename)):
        dictionary[key] = value
    imported_dictionary = dictionary
    return imported_dictionary

# Saves a dictionary as a file with csv    
def csv_export(dictionary, filename): 
    csv_file = csv.writer(open(filename, "w"))
    for key, value in dictionary.items():
        csv_file.writerow([key, value])

# Automatically adjusts column widths       
def auto_width(dictionary, min_len, max_len):
    key_len = len(max(dictionary.iterkeys(), key = len))
    value_len = len(max(dictionary.itervalues()))
    key_len = int(key_len)
    value_len = int(value_len)
    min_len = int(min_len)
    max_len = int(max_len)
    if key_len > max_len:
        key_len = max_len
    if value_len > max_len:
        value_len = max_len
    if key_len < min_len:
        key_len = min_len
    if value_len < min_len:
        value_len = min_len
    adjusted_key_len = key_len
    adjusted_value_len = value_len
    return (adjusted_key_len, adjusted_value_len)

# Prints to console keys, values in columns    
def print_formatted_dict(dictionary, min_len, max_len):
    # If no column width is specified the max item length will be used
    key_len, value_len = auto_width(dictionary, min_len, max_len)
    title_align = '{:^%s} {:^%s}' % (key_len, value_len)
    key_value_align = '{:<%s} {:<%s}' % (key_len, value_len)
    print title_align.format(key_title, value_title)
    for key, value in dictionary.items():
        print key_value_align.format(key, value)

# Raw_input question loop returning answers conditionally    
def ask(question, choices, true_if = None):
    answer = ''
    # Loops while answer not in choices
    while answer not in choices:
        answer = raw_input(question)
    # Returns bool if specified that answer need match only one of choices
    if true_if != None:
        if answer == true_if:
            return True
        else:
            return False
    # Returns any of the possible choices as answer
    ask_answer = answer
    return ask_answer

# Raw_input question joined to item string accepting any answer    
def ask_about(question, item):
    question = question + item + ': '
    answer = raw_input(question)
    ask_about_answer = answer        
    return ask_about_answer

# A loop for specifying keys to add, pop, or change key values
def change_dict(dictionary, min_len, max_len):
    changed = False
    change_answer = ''
    while change_answer != 'no':    
        change_answer = ask(change_question, yes_no_pop_display)
        if change_answer == 'yes':
            key_answer = raw_input(key_question)
            value_answer = ask_about(value_question, key_answer)
            dictionary[key_answer] = (value_answer)
            print_formatted_dict(dictionary, min_len, max_len)
            changed = True
        if change_answer == 'pop':
            key_answer = raw_input(key_question)
            dictionary.pop(key_answer, 0)
            print_formatted_dict(dictionary, min_len, max_len)
        if change_answer == 'display':
            min_len = ask_digit(min_len_question)
            max_len = ask_digit(max_len_question)
            print_formatted_dict(dictionary, min_len, max_len)
    changed_dictionary = dictionary
    return (changed_dictionary, changed)

# Return an answer only if it is a digit
def ask_digit(question):
    answer = ''
    is_digit = answer.isdigit()
    while is_digit == False:
        answer = raw_input(question)
        is_digit = answer.isdigit()
    ask_digit_answer = answer
    return ask_digit_answer

# Return a filename only if '.' is in the string
def valid_filename():
    filename_answer = ''
    while '.' not in filename_answer:
        filename_answer = raw_input(filename_question)
    returned_filename_answer = filename_answer
    return returned_filename_answer

# Save a file with csv_import()    
def save_dict(dictionary):
    filename_answer = valid_filename()
    csv_export(dictionary, filename_answer)

# Load a file with csv as a dictionary using a user specified filename
def load_dict(dictionary):
    filename_answer = valid_filename()
    dictionary = csv_import(filename_answer)
    loaded_dictionary = dictionary
    return loaded_dictionary

# A function to display, edit, and save dictionaries as .csv files
def edit_dict(dictionary = None, filename = None):
    # If nothing is passed a dictionary is loaded from default.csv   
    if dictionary == None and filename == None:
        default_filename = 'default.csv'
        dictionary = csv_import(default_filename)
    # If a filename is passed it is loaded as a dictionary
    if filename != None:
        dictionary = csv_import(filename)
    # The dictionary is displayed in columns
    print_formatted_dict(dictionary, min_len, max_len)
    load_answer = 'yes'
    while load_answer == 'yes':
        # The dictionary values may be changed by specifying key values
        dictionary, changed = change_dict(dictionary, min_len, max_len)
        if changed == True:
            # The dictionary may be saved if changes were made
            if ask(save_question, yes_no, 'yes'):
                save_dict(dictionary)
                print_formatted_dict(dictionary, min_len, max_len)
        # A new dictionary may be loaded with csv_import()
        load_answer = ask(load_question, yes_no)
        if load_answer == 'yes':
            dictionary = load_dict(dictionary)
            print_formatted_dict(dictionary, min_len, max_len)
            changed == False
    # The last dictionary loaded is returned
    edited_dict = dictionary
    return edited_dict

# Question strings
value_question = 'Specify '
key_question = 'Key to change/add: '
load_question = 'Load a file? '
change_question = 'Specify change (yes/no/pop/display): '
filename_question = 'Filename: '
save_question = 'Save file? '
min_len_question = 'Min column width? '
max_len_question = 'Max column width? '

# Integers
min_len = 15
max_len = 50

# Strings for formatting dictionary display
key_title = 'Key'
value_title = 'Value'

# List variable used by multiple functions
yes_no = ['yes', 'no']
yes_no_pop_display = ['yes', 'no', 'pop', 'display']

def main():
    edit_dict()

if __name__ == '__main__':
    main()

Here's sample usage...

      Key            Value     
gil             500            
Specify change (yes/no/pop/display): yes
Key to change/add: gil
Specify gil: 0
      Key            Value     
gil             0              
Specify change (yes/no/pop/display): yes
Key to change/add: sword
Specify sword: diamond
      Key            Value     
gil             0              
sword           diamond        
Specify change (yes/no/pop/display): pop
Key to change/add: gil
      Key            Value     
sword           diamond        
Specify change (yes/no/pop/display): yes
Key to change/add: a very long key a very long key
Specify a very long key a very long key: small value
              Key                    Value     
a very long key a very long key small value    
sword                           diamond        
Specify change (yes/no/pop/display): display
Min column width? 0
Max column width? 0
Key Value
a very long key a very long key small value
sword diamond
Specify change (yes/no/pop/display): display
Min column width? 0
Max column width? 50
              Key                  Value   
a very long key a very long key small value
sword                           diamond    
Specify change (yes/no/pop/display): no
Save file? yes
Filename: mystuff
Filename: mystuff.csv
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

In general, you've done a pretty good job breaking down the problem into functions.

You've written a comment describing every function. That's good, but you might as well write the comments as docstrings instead.


You have a probable file descriptor leak in csv_import() and csv_export(), because you call open() but not close(). An even better habit, though, would be to always use open() in the context of a with block.

Furthermore, csv_import() could be simplified by using a dict comprehension:

def csv_import(filename):
    with open(filename) as f:
        return {key: value for key, value in csv.reader(f)}

The auto_width() function seems suspiciously long, for what should be simple functionality. For one thing, everything is done twice: once for the keys, once for the values. That led to a copy-and-paste bug: value_len = len(max(dictionary.itervalues())) finds the length of the value that comes last in alphabetical order, rather than the length of the longest value. The int() conversions for key_len and value_len are superfluous. The int() conversions for min_len and max_len should have been done when reading / validating the user input.

Renaming the results to adjusted_key_len and adjusted_value_len is also unnecessary. In general, you have a habit of needlessly renaming variables just before returning.

def auto_width(strings, min_len, max_len):
    """Find the maximum length among an iterable of strings, but bounded by
    at least min_len and at most max_len."""
    longest = len(max(strings, key=len))
    return max(min(longest, max_len), min_len)

def print_formatted_dict(dictionary, min_len, max_len):
    key_len = auto_width(dictionary.iterkeys(), min_len, max_len)
    value_len = auto_width(dictionary.itervalues(), min_len, max_len)
    …

Many of your conditions are written as if something == None: … or if something != None: …. These comparisons should be written as

if something is None:
    …

if something is not None:
    …

Also, while is_digit == False: … and if changed == True: … would read better if written as

while not is_digit:
    …

if changed:
    …

The start of edit_dict() could be simplified by using default parameters effectively.

def edit_dict(dictionary=None, filename='default.csv'):
    if dictionary is None:
        dictionary = csv_import(filename)
    # The dictionary is displayed in columns
    print_formatted_dict(dictionary, min_len, max_len)
    …

Further down in edit_dict(), the changed == False looks suspicious. Surely, you meant to assign rather than compare. (Typically, such a flag would be called the dirty flag, to indicate that something has changed and therefore needs to be saved.)

if load_answer == 'yes':
    dictionary = load_dict(dictionary)
    print_formatted_dict(dictionary, min_len, max_len)
    changed == False
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.