patternpythonflaskMinor
Utility Class for CRUD Operations in a Flask App
Viewed 0 times
flaskoperationsapputilityforclasscrud
Problem
I'm new to Flask and Python and am trying to sort out the most pythonic way to modularize my utilities. I have
To explain more clearly the purpose of this utility is to retrieve records from the DB and serve the Views with the processed data. In response to some of the answers and comments I'd like to point out that although I'm referencing a Document and Business both of these are part of the same One-To-Many relationship and belong to the Business. (IE: In my app, the Business is referenced as a "Business Record" and therefore the
The following snippet is from my
```
class RecordManager(object):
def __init__(self):
self.__center = Center.query.get(session['center'])
self.__business = None
self.__record = None
@property
def id(self):
return self.__record.id
def all(self, archived=0):
return self.__center.businesses.filter_by(archived=archived).all()
def get(self, biz, doc=None, obj=None):
self.__business = self.__center.businesses.filter_by(bizId=biz).first_or_404()
if not doc == None:
return self.__business.info.documents.filter_by(id=doc).first_or_404()
if obj: return self.__business
return self.__business.info
def list(self):
documents = self.__business.info.documents.with_entities(Document.typId).all()
if not documents:
documents = [[-1,-1]]
return zip(*documents
models.py which have my sqlalchemy models and views.py which handles the routes, etc... My util.py basically handles the queries and data manipulation between the two but I want to know if I'm doing it right. I originally had a bunch of loose functions but I recently began compounding them into classes.To explain more clearly the purpose of this utility is to retrieve records from the DB and serve the Views with the processed data. In response to some of the answers and comments I'd like to point out that although I'm referencing a Document and Business both of these are part of the same One-To-Many relationship and belong to the Business. (IE: In my app, the Business is referenced as a "Business Record" and therefore the
RecordManager() class manipulates both Business Information (contained in the Business Table) and Business Documents (contained in the Documents Table). Additionally, the Business is part of a Many-To-Many relationship with the Center.The following snippet is from my
util.py:```
class RecordManager(object):
def __init__(self):
self.__center = Center.query.get(session['center'])
self.__business = None
self.__record = None
@property
def id(self):
return self.__record.id
def all(self, archived=0):
return self.__center.businesses.filter_by(archived=archived).all()
def get(self, biz, doc=None, obj=None):
self.__business = self.__center.businesses.filter_by(bizId=biz).first_or_404()
if not doc == None:
return self.__business.info.documents.filter_by(id=doc).first_or_404()
if obj: return self.__business
return self.__business.info
def list(self):
documents = self.__business.info.documents.with_entities(Document.typId).all()
if not documents:
documents = [[-1,-1]]
return zip(*documents
Solution
OOP
It seems you're accustomed to organizing your code into functions,
and you're new to organizing into classes.
When creating a class, the most important question to ask is what Abstract Data Type (ADT) are you trying to represent. Loosely defined, an abstract data type is a collection of data and functions that work with that data.
Here are some key points to guide you:
I recommend reading Chapter 6 in Code Complete, it should give you excellent ideas about designing classes well.
In your program, the main class appears to be
In terms of the above key points, this class is doing fine,
but only on the surface.
It looks like its purpose is CRUD operations,
but the problem is that it works with
It seems that these concerns should be separated,
perhaps into
Speaking of "Business"...
The purpose of these classes is unclear, I suggest to rethink, keeping in mind the key points I listed above.
Looking at the implementation of the methods of
this class knows too much internal details about
Chained calls like
See also: the Law of Demeter
Style
Python has an official style guide called PEP8.
It's a very interesting read.
Some of the most notable violations in the posted code:
It seems you're accustomed to organizing your code into functions,
and you're new to organizing into classes.
When creating a class, the most important question to ask is what Abstract Data Type (ADT) are you trying to represent. Loosely defined, an abstract data type is a collection of data and functions that work with that data.
Here are some key points to guide you:
- An ADT should have one very clear purpose.
- See also: Single Responsibility Principle
- The data of an ADT should not be accessed directly from the outside. Access an ADT only through its methods.
- See also: encapsulation, information hiding
- Example: imagine a
Fontinstance. The size of a font might be stored in pixels or in points. An implementation will probably use either of the two, but not both. If you ever have to dofont.sizeInPixels = 12, then you know too much about the implementation. The authors cannot refactor the class to usesizeInPointsanymore, without breaking all the code that accesses the internal data directly. If you enforce using only use methods, for examplefont.setSizeInPixels(12), the internal representation is hidden, and the author is free to change from pixels to points without affecting users.
- The method names of an ADT should use wording in the problem domain.
- Example: imagine an ADT in charge of an exchange rates table, that stores the rates table in a CSV file. If the method to load the table is called
loadRatesFile, the term "File" is bad in two ways: it reveals an implementation detail, and it's not in the language of the problem domain. Users of the ADT may find it confusing why it's talking about a "File", when they don't care where the table is coming from. Indeed it could come from anywhere. The author may want to replace the implementation to load the table from a database or a web service.loadRatesTablewill be a better name in the problem domain, and the implementation will be free to change, without rendering the method name meaningless.
I recommend reading Chapter 6 in Code Complete, it should give you excellent ideas about designing classes well.
In your program, the main class appears to be
RecordManager.In terms of the above key points, this class is doing fine,
but only on the surface.
It looks like its purpose is CRUD operations,
but the problem is that it works with
Business and Document objects.It seems that these concerns should be separated,
perhaps into
BusinessManager and DocumentManager classes.Speaking of "Business"...
Business, Center, CenterBusiness seem wishy-washy, meaningless names.The purpose of these classes is unclear, I suggest to rethink, keeping in mind the key points I listed above.
Looking at the implementation of the methods of
RecordManager,this class knows too much internal details about
Business and the other classes. For example:def get(self, biz, doc=None, obj=None):
self.__business = self.__center.businesses.filter_by(bizId=biz).first_or_404()
# ...Chained calls like
self.__center.businesses.filter_by are a sign of too tight coupling. This class knows that Center has a field called businesses. This detail should be hidden in Center, and the filter should only be accessible through a method of Center, something like:self.__business = self.__center.first_business(biz)See also: the Law of Demeter
Style
Python has an official style guide called PEP8.
It's a very interesting read.
Some of the most notable violations in the posted code:
- Use single
_prefix for private variables instead of double__
- Instead of
not doc == None, should beis not None
- Likewise, instead of
doc == None, should bedoc is None
- Instead of
if obj: return self.__business, break the line after the:
Code Snippets
def get(self, biz, doc=None, obj=None):
self.__business = self.__center.businesses.filter_by(bizId=biz).first_or_404()
# ...self.__business = self.__center.first_business(biz)Context
StackExchange Code Review Q#115433, answer score: 2
Revisions (0)
No revisions yet.