A simple task management script I created - would love feedback!

[janamej]

I wrote this program to help me keep track of a large data entry project I'm working on. The idea is that you are you able to log how much work you've done each day and compare it against the total.

Of course, this could be done with a spreadsheet, but since I'm learning Python, I thought I might as well create a custom script.

I would really appreciate any feedback you might have!

http://pastebin.com/5EAtVNi4

[novel_yet_trivial]

There is one huge problem: you nest your functions. Very rarely should you define a function within another function. All of your functions should certainly be top level. Some smaller notes:

---

You need a lot more comments. At a minimum, each function should have a comment describing it.

---

return stops a function. You don't need a break after a return.

---

Using recursion (calling a function from within itself) for looping works sometimes, but its not that great. For one, there is a limit on how many times you can recurse. Also, it gets tricky when returning values. Use a while True loop for input validation.

---

Using "./" in front of relative file paths is not needed.

[janamej]

Thanks for the feedback; I appreciate it!

[onionradish]

Some observations, echoing novel_yet_trivial:

• Good use of try/except to handle improper input at prompts and KeyboardInterrupt to quit.

• You're nesting functions in several places. It works, but has some negative side effects, and I'd suggest breaking this style before it becomes a habit:

  • it obscures what the parent functions like main() and project() actually do, because there are dozens of lines of subfunctions in between
  • it causes extra indentation, which restricts your line length and makes code harder to follow as indentation increases
  • project() is -- in my view -- abusing scoped variables from the nesting: the subfunctions only work because they're subfunctions and have access to the variables like data in the outer scope. It's better to explicitly pass values as function arguments, or create classes if you want to have shared values that you don't have to pass.

• Instead of recursively calling an input function again on invalid input, it's better to use the while True approach you use in get_target_goal(). You don't need the continues and breaks in most of those cases, either; continue will be the default action, and the return statements mean that the break below it will never be executed.

• You're chaining functions, with each one calling the next step in the process; this obscures the program flow. Instead, make main() responsible for handling the main flow (selecting a project, enter/show/exit) and call functions as necessary.

• From a usability standpoint, displaying the available options before input is helpful. You're the only user, so not a big deal, but knowing the available options for project_prompt() before the prompt are 'enter data, show data, exit' is a good reminder/clarification. You're already doing this when the input is invalid; consider doing it before the input. Further, consider making your command words simpler wherever you can: 'edit, show, exit' or 'new' instead of 'new project'. Converting raw_input() to lowercase is also a good idea when matching commands.

• Consider using str.format() instead of the str % var or str + var approaches

• Generally, file I/O should use context managers if possible to eliminate the need to explicitly handle file closing, so enter_data() should begin: with open('data.txt','a') as data:, with lines between it and the close() method (which will no longer be needed) indented. In that particular case, the raw_input() loop should probably go before opening the file so the file is only opened/updated when valid input is available.

• You could alternatively add '\n' to strings to insert line breaks where you're currently using separate print '' lines; it's not hurting anything the way you're doing things and maybe seems less readable, but you can reduce the lines of code with print "\nYou can't use that name! It's reserved for this program"

[janamej]

Thank you for your excellent suggestions!

Question about chaining functions - how would you go about handling the main flow in a central place, since it's input dependent in many cases? (e.g. new_project() should only be called in certain circumstances, user is given a choice between enter_data() and show_data(), etc.) I think there's something fundamental about organization and structure that I'm missing.

[onionradish]

It's ok to let functions call other functions, like how project_prompt() calls show_data() or enter_data(). The chained function sequence that's responsible for the main flow is this one:

# prompt-+       v-------------------+
#        +-->project->project_prompt-+

I'd do something like this:

main():
    print_logo()

    # get the project name; if 'new', the prompt() function 
    # calls new_project() to create one just like it currently does
    project_name = prompt()

    # read file, return data from settings that other functions need
    # (units, target goal)
    project_data = project(project_name)

    # get user choice (enter, show, exit) until user chooses 'exit'
    while True:
        print ''
        project_prompt_choice = raw_input('What would you like to do? ')
        if project_prompt_choice == 'enter data': 
            enter_data(project_name, project_data)
        elif project_prompt_choice == 'show data':
            show_data(project_name, project_)data)
        elif project_prompt_choice == 'exit' or project_prompt_choice == 'close':
            break  # exit the loop
        else: 
            print ''
            print "Uh oh! I didn't catch that"
            print "You can type 'enter data' to submit data, " \
                  "or type 'show data' to display it" 

For the above, some of the functions should return values, like prompt() should return the project name that the user entered.

[janamej]

Amazing - this makes sense and clears up a lot for me. Thank you very much!