blreams / collegefootballpick10

Football pool web app
4 stars 2 forks source link

database.py uses ambiguous get_game() function #114

Closed blreams closed 8 years ago

blreams commented 8 years ago

In database.py, there are the following imports: from pick10.models import * from calculator import * The ambiguity is that both of these modules (models and calculator) have a function called get_game(). I believe that python's import rules are such that the first one it encounters will take precedence and the latter will be ignored. I confirmed this by doing the following in a python shell:

>>> from pick10 import database
>>> database.get_game.__module__
'pick10.models'
>>>

I would like to do a refactor of the code to remove all cases where there imports that use asterisk to import everything. This is a bad coding practice that makes it difficult to tell where objects get imported from.

blreams commented 8 years ago

I've completed the vast majority of the refactoring to get rid of "import *". The only code that I didn't really touch were the functional_tests. Mainly, I didn't touch these because I haven't run them yet to baseline what is working and what isn't. Best Practice for refactoring is to make sure the code passes all tests before making changes. Then refactoring should not change any functionality. After refactoring is complete, all tests should pass as is. Refactoring should never be mixed with functional changes.

I'll spend some time trying to get the functional tests to run somewhere (probably on my linux server). But if I can't, I at least feel better that I did run all of the unittests.

blreams commented 8 years ago

I'm leaving this open until I have deployed this code to pythonanywhere. I didn't want to do that until after we are past the pick deadline. It will most likely happen sometime this weekend.

blreams commented 8 years ago

I was finally able to get functional tests running and I'm glad I did. I found a couple more issues with the refactored code (where I didn't directly import a class from models.

blreams commented 8 years ago

Closing this issue, now that I have deployed the refactored code to pythonanywhere.