current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I'm now looking for feedback on v2.0 of this program instead.

I'd like some feedback about my code. What bad habits do I have? What advice could help me write more Pythonic?

I'm trying to write a console application and want a tool to display, update and save settings in a dictionary.

def file_as_string(file_name):
    with open(file_name, 'r') as opened_file:
        string = opened_file.read().replace('\n', '')
        return string

def csv_as_dict(file_name):
    file_name = file_name + '.csv'
    import csv
    dictionary = {}
    for key, value in csv.reader(open(file_name)):
        dictionary[key] = value
    return dictionary

def print_formatted_dict(dictionary, key_title , value_title):
    print "{:^30} {:^15}".format('%s', '%s') % (key_title, value_title)
    for setting, value in dictionary.items():
        print "{:<30} {:<15}".format(setting, value)

def generic_prompt(prompt):
    value = raw_input(prompt)
    return value

def prompt_for_item(prompt, item):
    prompt = prompt + ' ' + item + ': '
    value = raw_input(prompt)        
    return value

def change_dict_value(dictionary, prompt, item):
    value = prompt_for_item(prompt, item)
    dictionary[item] = (value)
    return dictionary

def csv_save_dict(dictionary, file_name): 
    file_name = file_name + '.csv'
    import csv
    csv_file = csv.writer(open(file_name, "w"))
    for key, value in dictionary.items():
        csv_file.writerow([key, value])    

def change_dict(dictionary):
    specify_question = 'Specify'
    change_question = 'Change settings? '
    setting_question = 'Setting to change: '
    save_question = 'Save settings? '

    change_answer = generic_prompt(change_question)
    while change_answer not in yes_no:
        change_answer = generic_prompt(change_question)
    while change_answer == 'yes':
        change_item = generic_prompt(setting_question)
        while change_item not in dictionary:
            change_item = generic_prompt(setting_question)
        dictionary = change_dict_value(dictionary, specify_question, change_item)
        print_formatted_dict(dictionary, key_title, value_title)
        change_answer = generic_prompt(change_question)
        while change_answer not in yes_no:
            change_answer = generic_prompt(change_question)
        if change_answer == 'no':
            save_answer = generic_prompt(save_question)
            while save_answer not in yes_no:
                save_answer = generic_prompt(save_question)
            if save_answer == 'yes':
                filename_answer = generic_prompt(filename_question)
                csv_save_dict(dictionary, filename_answer)
    return dictionary

def load_dict(dictionary):
    load_answer = generic_prompt(load_question)
    while load_answer not in yes_no:
        load_answer = generic_prompt(load_question)
    if load_answer == 'yes':
        filename_answer = generic_prompt(filename_question)
        dictionary = csv_as_dict(filename_answer)
        print_formatted_dict(dictionary, key_title, value_title)
    if load_answer == 'no':
        pass
    return (dictionary, load_answer)

def load_change_loop(dictionary, load_answer):
    while load_answer == 'yes':
        dictionary = change_dict(dictionary)
        dictionary, load_answer = load_dict(dictionary)
    return dictionary

def main():
    default_dict = csv_as_dict(default_settings)
    print_formatted_dict(default_dict, key_title, value_title)
    dictionary = load_change_loop(default_dict, load_answer)
    return dictionary

#string variables
default_settings = 'default_settings'
load_question = 'Load a dictionary? '
filename_question = 'File name: '
key_title = 'Setting'
value_title = 'Value'
load_answer = 'yes'

#list variables
yes_no = ['yes', 'no']

main()

The console with highlighted user input looks like this: Console

The program begins by reading a default_settings.csv file, mine looks like this: Excel

share|improve this question
    
A CSV up file and your output are both just text; please provide them as such, rather than as screenshots. –  jonrsharpe Aug 12 at 7:15

3 Answers 3

up vote 5 down vote accepted

It's a pretty good program for a beginner.

A few points:

Put your import csv once at the top of the file, not in the functions that use csv. Importing is best thought of as an action at the start of your program, not as something that happens while performing an action.

You do this a lot:

while load_answer not in yes_no:
    load_answer = generic_prompt(load_question)
if load_answer == 'yes':

Anytime you find yourself repeating code, it'd be helpful to write a function instead. Something like:

   def ask_boolean_question(prompt):
       answer = ''
       while answer not in yes_no:
           answer = generic_prompt(prompt)
       return answer == 'yes'

Now your code can simply say:

  if ask_boolean_question(load_question):

And avoid repeating the loop everytime you ask a question.

Your program doesn't make its structure very clear. Given what your program does, your main function should read something like:

def main():
    manipulate_settings(default_settings)
    while ask_boolean_question(load_question):
         filename = generic_prompt(filename_question)
         manipulate_settings(filename)

Note that by reading this, you can see the program processes first the default settings, and then as many other settings files as you have inclination to use the program.

share|improve this answer
up vote 2 down vote

You have taken quite an odd approach to splitting this up into functions. For example, generic_prompt is totally pointless;

value = generic_prompt(prompt)

adds nothing over:

value = raw_input(prompt)

and just makes your code harder to read. And yet, as Winston points out, you have repeated the code for a yes/no input over and over again. prompt_for_item (value = raw_input('%s %s: ' % (prompt, item))) and change_dict_value (dictionary[item] = raw_input(...)) also seem suspect.

Both change_dict and change_dict_value mutate their argument and return it. This is unconventional; typically, functions that mutate arguments return None, implicitly or explicitly. 

def csv_save_dict(dictionary, file_name): 
    file_name = file_name + '.csv'
    ...

It seems odd to expect the filename passed to a function to save a CSV to not include the extension. You should check this when the user inputs the filename, or at the very least guard this change with:

if not file_name.endswith('.csv'):

(although note that it's not actually necessary to call the file a .csv; the user may have specified a different extension)

Also, I would call this save_dict_as_csv, or just export_dict (now you don't have to rename if you switch from csv to e.g. pickle).

Some of your "constants" (usually UPPERCASE_WITH_UNDERSCORES in Python) seem oddly-placed. For example, key_title and value_title could be moved inside print_formatted_dict, removing the need for two arguments that only pass constants along. You could also make an input function get_filename, called for both importing and exporting, where filename_question could be moved to (this would also allow you to later add checking that the file in question does/doesn't exist, or switch to a file chooser). In both cases, I would just have a literal string, rather than separately define it, e.g.:

print "{:^30} {:^15}".format('%s', '%s') % ("Setting", "Value")

Your code has no explanation whatsoever. At the very least, you should add in some docstrings (see PEP-257) outlining what each function does.

share|improve this answer
up vote 2 down vote

It is good practice to follow code style guidelines from PEP8 and Google if you want to write more Pythonic code.

To be more specific:

    dictionary = change_dict_value(dictionary, specify_question, change_item)

Is to long, you can write it like this:

    dictionary = change_dict_value(dictionary, specify_question,
                                   change_item)

Comments should have space after #, so:

# String variables

rather than:

#string variables

I'm finding writing docstrings for each and single function I write a very good practice. You can read more about docstrings formating in PEP257.

It is also common to write:

if __name__ == '__main__':
    main()

rather than

main()

It helps to reuse once written code by creating new modules and packages, while keeping ability to run script directly. For the reference about packages and modules read Python documentation about them.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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