1
\$\begingroup\$

I made a rock climbing simulator game for fun. Below is a function from the program, which works. But I can't help but think it could be refactored or more pythonic.

Namely, I open the data with csv.DictReader and read through the data, which add to a blank dictionary called route_info. Is this step necessary?

The function has a with-open block, which opens a .csv file, but a link to the data is here.

import csv
import time


routes_climbed = []
pitches_climbed = 0

def choose_route():
    """After deciding about the weather, you come here and choose a 
    route to climb. Reads list of routes from .csv file with names, 
    grades, and pitches."""

    global routes_climbed, pitches_climbed

    route_info = {}

    with open('seneca_routes.csv', 'r', encoding='utf-8-sig') as f:     #weird encoding from excel?
        reader = csv.DictReader(f, ('route_name', 'route_grade', 'pitches'))
        print("Choose a route to climb from the following list of classics.")
        print()

        for row in reader:
            print(row['route_name'],row['route_grade'])
            route_info[row['route_name']] = row['pitches']

    route_choice = input("Which route do you want to climb? : ")

    if route_choice in route_info.keys():
        routes_climbed.append(route_choice)
        pitches_climbed += int(route_info[route_choice])
        climb_route(route_choice)
    else:
        print("Incorrect route name")
        time.sleep(3)
        choose_route()

choose_route()
\$\endgroup\$
2
\$\begingroup\$

Some suggestions:

  1. Pass the code through at least one linter such as pycodestyle or flake8 to get more idiomatic Python.
  2. Don't use global. Returning two values, while ugly, is preferable to having global state.
  3. You can add \n to the end of a string to print a newline, avoiding empty print()s.
  4. Use argparse to rather than input() to make the script … well, scriptable.
  5. Each route should probably be an object rather than a disparate set of just stuff, even though these objects will have no methods for now.
  6. Using a main method as the entry point for the functionality would make the script usable by other scripts.
\$\endgroup\$
  • \$\begingroup\$ Thanks. I'm thinking about how to not use global. Would a class be a better way to do this? What would you suggest? \$\endgroup\$ – Erich Purpur Oct 26 '18 at 19:55
  • \$\begingroup\$ The very simplest thing you could do is simply move the declaration of the current globals into your function - they aren't referenced outside the function, after all. This code does look like it could be made clearer through OO, but that could easily be a separate refactoring. \$\endgroup\$ – l0b0 Oct 26 '18 at 20:14
  • \$\begingroup\$ I can do that, however, I have different functions (not included here) that are affecting the global variables. For example, once I choose a route I go to the climb_route() function. In climb_route() I ask "Do you want to climb another", if so then I return to choose_route(). So I'd be overwriting my routes_climbed and pitches_climbed variables. Just so I know better in the future, should I include my entire program in code review? \$\endgroup\$ – Erich Purpur Oct 26 '18 at 20:32
  • \$\begingroup\$ It depends what you want to be reviewed. I just see two globals which clearly don't need to be; with the rest of the code it might be much more obvious how to refactor it. \$\endgroup\$ – l0b0 Oct 26 '18 at 22:39

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.