HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

A module to display, update and save a dictionary as .csv v2.0

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
updatecsvmodulesaveanddictionarydisplay

Problem

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, choi

Solution

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

Code Snippets

def csv_import(filename):
    with open(filename) as f:
        return {key: value for key, value in csv.reader(f)}
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)
    …
if something is None:
    …

if something is not None:
    …
while not is_digit:
    …

if changed:
    …
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)
    …

Context

StackExchange Code Review Q#59975, answer score: 4

Revisions (0)

No revisions yet.