Refactor controller file

[davidskalinder]

@alexhanna I think I might need your input here. I think issue #15 should be solved by putting in the ordered_load function from https://stackoverflow.com/a/21912744. The question is where to put it. AFAICT at the moment the controller is structured like this:

1. Module imports
2. Application and login manager creation
3. Definitions of (global?) variables (v2, yes_no_vars, meta_solr, etc.)
4. Helper functions (including some with and some without @app decorators)
5. "App setup" functions
6. "Coding pages" functions
7. Pagination class (many of whose methods have a @property decorator)
8. paginate function
9. url_for_other_page function
10. A variable definition (app.jinja_env.globals['url_for_other_page'] = url_for_other_page)
11. code2queue1 function
12. "Coder stats" functions
13. "Internal calls" functions
14. "Admin tools" functions
15. if name == 'main'

I think it'd be nice to tidy that up a bit in general, but in particular at the moment this structure is making it hard to use functions when defining variables. Variables defined from YAML files are currently in section 3, but at that point none of the functions have been read in; so if I want to use the ordered_dict function to get a yaml file into a variable, I either have to shoehorn it in before the variable definitions (where no other functions live), or move the variable definitions after the functions.

My natural inclination is to put all the functions first and all executed code afterward, but it seems that the @app and @lm decorators depend on app and lm being set above the decorated function calls. So I guess to follow my inclination would mean defining app and lm, then all the functions, and then the rest of the variables, which seems ungainly and error-prone.

Other solutions I can think of would be to move undecorated helper functions up (which also seems weird because then the variable definitions are in the middle); to move only the helper functions to a separate file and import them; to move lots of stuff (everything?) to separate files and import it; or to do some clever thing so we don't have to define a bunch of what I think are global variables (which make me a little nervous?) in sections 3 and 10.

I assume all this is a good candidate for upstream since it's ultimately just refactoring (although I assume we'd want the ability to fix yaml list order in upstream as well, which depends on this); and since this is partly a matter of code style, and since it'd move a lot of stuff around, I wanted to see what you reckoned is best?

[alexhanna]

I don't have strong opinions on refactoring. It's not good style that everything lives in mpeds_coder.py to begin with. I think moving all the helper files to a library file for import is a good idea. Other than that, if you have a proposed structure based on some standard or best practice, I'm all ears.

[davidskalinder]

Okay, so for now I'm going to move the undecorated helpers to a separate file and import that. Further refactoring seems like a good idea but requires further research since there appear to be copious recommendations on the subject (for my reference when I get to it):

https://softwareengineering.stackexchange.com/questions/187403/import-module-vs-from-module-import-function

https://blog.miguelgrinberg.com/post/the-flask-mega-tutorial-part-xv-a-better-application-structure

https://dev.to/aligoren/how-i-structure-my-flask-apps-3eh8

https://www.patricksoftwareblog.com/structuring-a-flask-project/

https://pythonise.com/series/learning-flask/flask-application-structure

https://stackoverflow.com/questions/14415500/common-folder-file-structure-in-flask-app

https://exploreflask.com/en/latest/organizing.html

https://flask.palletsprojects.com/en/1.1.x/patterns/packages/

[davidskalinder]

Having started in on this, I realize that unless I'm missing something, this is going to be a slow and painstaking process every step of the way because for every function (every line of code, really) that I move I'm going to need to check for its dependencies and also move them. This might include both module imports and global variables (and I don't know if Python allows globals to be used in functions if the function is defined before the global is?). Complicating this further is that all pages that depend on every function would need to be checked to make sure nothing unexpected goes wrong.

A (bad) workaround would be to simply repeat all of the imports from the controller in every other file, which just kicks the problem down the road. (And moreover, testing this just now didn't even work, though I'm not sure why.)

So unfortunately I think this isn't really feasible before the live launch. Issues that depend on this one will have to just shoehorn in whatever's necessary for now, which will make the controller even messier until we have time to clean it up properly. Rats.

[davidskalinder]

ordered_load function from #15 has been duly shoehorned. The controller now looks like this:

1. Module imports
2. ordered_load function
3. Application and login manager creation
4. Definitions of (global?) variables (v2, yes_no_vars, meta_solr, etc.)
5. Helper functions (including some with and some without @app decorators)
6. "App setup" functions
7. "Coding pages" functions
8. Pagination class (many of whose methods have a @property decorator)
9. paginate function
10. url_for_other_page function
11. A variable definition (app.jinja_env.globals['url_for_other_page'] = url_for_other_page)
12. code2queue1 function
13. "Coder stats" functions
14. "Internal calls" functions
15. "Admin tools" functions
16. if name == 'main'

[davidskalinder]

A partial solution that occurred to me while reading https://docs.python-guide.org/writing/structure/#modules : convert all the from x import y statements to import x and replace all calls to y with calls to x.y. Figuring out the dependencies of each component will still require checking every instance of ., but that's a lot easier.

Of course that still leaves the globals, which might simply have to be checked at each level of indentation, then grouped into the same place and replaced one by one with something more sensible.

But after all of that, file-splitting should be much less fragile...?