This page contains a summary of a design review of Makahiki done in March, 2012.
- What is the appropriate level of object orientation? Should every module be implemented via classes? If not, how do we decide when to use object orientation and when to not use it?
- Unit tests need to be reviewed. It appears that some test classes are not even invoked.
- When is it appropriate to include methods in init.py?
- When is it appropriate to included methods in urls.py?
- When should a feature be part of management.commands, and when should it be an external, stand-alone script?
After completing this review of the Makahiki 2 codebase, I am struck by the realization that it does not yet feel like a “framework” or “game engine” to me.
For me, the defining feature of a domain-specific framework is that domain entities have a “first class” status within the framework. In our case, I view at least the following domain entities as requiring “first class” status for us to have a framework:
- Authentication methods
- Scoring systems
Currently, I do not view any of these entities as having first class status. For that to be the case, we must have an easy way to perform the following:
- Create an instance of the entity.
- Ensure that all instances of the entity have an appropriate, common structure.
- Be able to determine how many instances of the entity exist.
- Be able to enable or disable an instance of the entity.
- Be able to easily extend the system to support new instances of the entity.
Typically, a framework supports the first class status of domain-specific entities through an object orientation with classes that define the common structural features and the internal state, and instances reflecting all the definitions of the entities. Some first class entities require persistent state (players, teams), others might not (authentication systems).
What I am instead seeing in Makahiki at present is a basically a design in which a set of methods are triggered by http requests, make calls to an underlying database, and then return dictionaries of property-value pairs to pages for display. (See all of the views.py files for examples.) This is simple, but not abstract. It’s very “flat” as an architecture. For Makahiki to become a framework, I think we must create and manipulate domain-specific entities.
Click on the section title to go to corresponding documentation.
1. Does this provide facilities for memcached management?
(Yongwen) No. currently it only provide utilities to invalidate the specific cache. here are some ideas for improvement:
- Use first level class CacheMgr instead, extended from Manager base class
- Public methods:
- items() - list all cache items
- get() - get or set cache item
- invalidate() - invalidate the item
- ? restart() - restart cache after the configure
- ? configure(conf) - programmatically config the cache
- extend from base class: info() - display cache info and config
(Discussion results) Can remain as a module (since no state, no instances). Module will provide an abstraction layer over underlying cache management system. All calls go through this module. It can provide additional helpful utility methods to support common operations.
2. For Makahiki2, we need a more flexible mechanism for authentication, including CAS, OpenDirectory, and “manual”. Should the authentication manager implement support for multiple authentication schemes? How will this interact with the user interface for authentication?
(Yongwen) Yes, currently provides CASBackend model and login UI for Django managed authentication. More backend model could be provided to support AD, or others.
See authentication sequence email thread for UI related to multiple auth scheme.
(George) It seems to me that “manual” is always present, but we have plugins that take over this functionality. I found a LDAP plugin that should handle open directory at http://code.google.com/p/django-ldap-groups/. Django also supports pluggable authentication backends.
(Discussion) Not sure if a class is needed (“natural” implementation may not require state or multiple instances). Could be implemented as a module and hardwire all supported authentication schemes. (i.e. authentication might turn out to not be “pluggable” due to complexity of implementation. Instead, we just document what is needed to do in the code to add support for a new authentication scheme.).
3. Should we have a set of prepackaged log file analyses? If so, where do they go? scripts/? admin interface? management command? (More generally, when should something be a script vs. a management commmand vs. an admin form?)
(Yongwen) Heroku will have difficulty to write to local filesystem. so, log will need to be stored in DB. It will serve as the data source for analytics widget. There might not need any prepackaged log file analyses.
(George) I think it could be packaged in with mission control, since analyses could be displayed there. Mission control could have an export interface (dump to CSV?) and possibly a management command to do it on the command line.
(Discussion) Some combination of mission control and management command is good. Mission control is best, since doesn’t require command line access.
4. Model fields should be documented.
5. Model hardwires the point systems in use.
6. Management commands to load and reset users need unit tests.
Management commands is a simple wrapper to first level class methods.
7. Score data (referral bonus, points) directly in model.
(Yongwen) Could be moved to score_mgr.ScoreMgr.
8. Does not appear to support definition of new scoring systems (such as ‘gallons’ for water).
(Yongwen) Score_mgr currently only The handles point management and ranking for points. Energy ranking is handled in energy_scoreboard widget. Energy and Water was considered as widget components and un-pluggable.
9. Should the score manager have an internal data structure containing the current state of all scores for all users and teams that is queried by modules? Or should each player have an instance of a scoring system that provides their own personal data?
(Yongwen) The first approach may be less effort with the current model, maybe better performance?
(George) I prefer the idea of each player/team having an instance of a scoring system. Seems more “relational” and cuts down additional database queries.
10. Where should one specify the organizational logo that goes in the header bar?
(George) Logo should be specified in the settings? Seems like it should be placed in the images folder. Maybe just have them put a file named “logo.png” in there?
11. Should competition_point_label be provided by scoring_mgr? 12. Should competition_team_label be provided by team_mgr? 13. Should cas_server_url be provided by auth_mgr?
(Yongwen) They are the user specified settings. The goal is to place them all in one place for better admin.
Another approach is to provide an interface in the base class Manager to provide the settings for individual managers and the admin interface will inspect and aggregate them for user input. Could be doable.
14. Lots of settings defined in init.py. Is this appropriate?
(Yongwen) Move methods in init.py into first-level class ChallengeMgr. In general, we could refactor all init.py methods into first-level classes, so all methods are class based. same things to views.py (refactor to class-based view).
(George) Constants and such should be fine in init.py. I’ve seen constants in there.
15. The tests.py file does not appear to be invoked during testing. Is the indentation wrong?
(Yongwen) Should be fixed thanks to George. will double check.
16. Do we want to hardwire methods to get a particular scoring system (points)? In the case of EWC, the “team” will also have a score related to gallons and kWh.
(Yongwen) Should not need the methods about the point or score. The view layer could query the score_mgr for such info.
17. The views module hardwires the address for admins.
(Yongwen) Should be part of the settings. Probably it is better to implement a common settings interface both for managers and widgets, so that the admin interface could inspect and aggregate the settings for all.
18. Currently we only have three possible badges. That seems lame; can we think of more?
(Yongwen) Ten is probably is the minimum. The framework should provide admin interface to add more:
- define rules for badge
- upload badge icon or use default icon
(George) Certainly needs improvement. One idea is to have category badges for the SGG, but it’d have to be generic since the categories are specified by content creators, not us.
19. Should the “manual” energy goal widget be a variant of this module, or a separate widget (apps.widgets.manual_energy_goal). Perhaps even more interestingly, since EWC will have a water challenge, maybe the manual widget should be able to be instantiated for either water or energy?
(Yongwen) The current design is to use the energy_goal model to store the data locally, either from watt depot (via external updater) or manual input. With Water in the picture, we could create a common goal model to be extended into energy, water, etc.
(George) I think the manual energy goal widget should be a configuration option. Although, if the energy goal is looking at a database table for information, does it matter how that table is populated?
20. It’s not really clear how/when energy goal points get awarded. Is there a periodic script that gets run each night? Where is that code? Can we put it in this module?
(Yongwen) The award_goal_points methods is for this purpose. There is a nightly script that simply calls this methods. Need testing.
(George) There is a script that checks. It was bundled in with another script, but that can be moved in here.
21. This widget appears to save energy data locally (as part of the model). Is this a change from Makahiki 1? Do we need to be persisting this data, or can we just keep it in-memory?
(Yongwen) A redesign from makahiki1. The thought is to remove dependency from GDATA and run updater to sync data from watt depot. It seems possible for query watt depot in real time without persistence.
(George) I guess we don’t need to persist it. That’s what WattDepot is for.
22. What does the admin interface to this actually accomplish? (Similar question for other energy widgets?)
(Yongwen) To support manual data input.
23. Three functions in init.py. Can these be moved elsewhere?
(Yongwen) Could be moved into first-level widget class “NotificationWidget”. Essentially, we will create a first-level class for all widgets exending from base class “Widget”, with the following common interface:
- info(): descriptive info for this widget
- settings(): any settings, both hardwired and user specified.
- enabled(): user changeable from admin
(George) They should be moved elsewhere if possible.
24. For consistency with new SGG terminology, should this be “popular_actions”?
(George) Actions for sure.
25. Should the management command for raffle picking and form printing move to the raffle widget?
(Yongwen) Yes, it may be enhanced as a admin interface functionality.
(George) Seems reasonable to have raffle related stuff there.
26. Should the “utility” functions be in init.py? More generally, should this module be more object-oriented?
(Yongwen) Implements the QuestWidget class.
(George) The functions can be moved elsewhere. As for making it more object oriented, maybe? The functions are directly mapped to predicates that are entered in the admin interface. As long as it doesn’t cause changes in the way they’re entered in the admin interface, I’m okay with it.
27. Shouldn’t the scoreboard widget refer to the score manager for data?
(Yongwen) Essentially it already does. The view method could be cleaned up.
28. Can this code can be restructured and simplified? Lots going on in init.py.
(Yongwen) Yes. Could implement SmartgridWidget and SmartgridView class.
(George) Definitely needs some cleaning up.
29. The team_members widget imports player_mgr but nothing from team_mgr. This seems confusing. Is it correct?
(Yongwen) The code uses team_id directly to query all player objects with the same team_id. It may not seem object-oriented by using the db model directly.
The object-oriented way could be:
- get the TeamMgr object from player
- call TeamMgr.members()
It seems to be a good idea to encapsulate the model query, so that if model changes, only the first-level class implementation needs to be changed accordingly. The external interface (views, interaction with other objects) could remain the same.
When to use object orientation? We will use OO when it is useful to encapsulate state and behavior, and when the class will have multiple instances. Otherwise, we will continue to use module-based design. Note that if we are creating multiple instances with internal state, we must support concurrent access, a problem we do not have when maintaining all state in the underlying database. OO seems most applicable for representing relatively static entities that are initialized at the start of execution and then do not change, such as teams.
Testing. We still need to review testing in more detail. We need both unit tests and integration (i.e. selenium) tests.
What should go in init.py? As a general rule, only a doc string should go in init.py.
Mission control, then admin interface, then management command, then script As a general rule, this sequence indicates the preferred location for administrative functions. Prefer online access over command line access, and avoid scripts. Note that it can be useful to implement a management command and then provide access to it via mission control.
The cache manager. Implement as a module, not a class. All other code will go through this module to access caching.
The authentication manager. We do not currently understand the design issues with various authentication structures well enough to decide upon the most appropriate “pluggable” architecture, if any. For now, simply implement additional authentication in the most convenient way possible, then we will review again.
The log manager. This manager should support storage and analysis of data that is later retrieved by the mission control page and/or management commands to provide “real time analytics” regarding the progress of the competition.
Document all model fields. Enables Sphinx to better document the system.
The score manager. The Score Manager must support definition of new scoring systems, and must encapsulate access to all score data (unlike Makahiki 1 where score data was hardwired in each user). Yongwen and George differ on whether there is a single Score Manager supporting all scores, or whether there should be multiple Score Manager instances, one per player and/or team. We must discuss this question further.
The Challenge Manager. We need a new module called the Challenge Manager that provides access to all details regarding the configuration of the current competition. As this is a singleton, it is best implemented as a module, not a class. Clients should use this instead of the settings module.
Badges. We should discuss whether or not the badge game mechanic is appropriate for Makahiki.
Energy goal game. There are at least four design questions with the energy goal game:
- How to eliminate the need for gdata spreadsheets? While some of the spreadsheets (baselines) will be persisted, others (latest power data) do not need to be persisted.
- The second question is: How to implement the manual DEGG? Options include: implement as a variant on the current “automated” DEGG; implement as a separate widget; or implement as a generic widget that can be specialized to water, etc.
- How to support A-B testing? We might want to be able to test whether the “goal” vs. “budget” representation yields different behaviors.
- How should the daily computation of award points be implemented? It was a script. Can we move it up the food chain (management command, admin interface, etc.)?
The team manager. Currently, code directly accesses the database for team data. It seems that code should instead be accessing the team manager module.