Utility Class for CRUD Operations in a Flask App

Question

I'm new to Flask and Python and am trying to sort out the most pythonic way to modularize my utilities. I have 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)[0]

    def store(self, form):
        if hasattr(form, 'bizId'):
            if form.id.data == 'new':
                self.__record = Document(typId=form.type.data, bizId=form.bizId.data,
                                         expiry=form.expiry.data)
                db.session.add(self.__record)
            else:
                self.__record = self.get(form.bizId.data, form.id.data)
                self.__record.expiry = form.expiry.data
        else:
            if form.id.data == 'new':
                assoc = CenterBusiness()
                assoc.info = Business(typId=form.type.data, name=form.name.data,
                                      contact=form.contact.data, phone=form.phone.data)
                self.__center.businesses.append(assoc)
                db.session.add(self.__center)
                self.__record = assoc.info
            else:
                self.__record = self.get(form.id.data)
                self.__record.name = form.name.data
                self.__record.typId = form.type.data
                self.__record.contact = form.contact.data
                self.__record.phone = form.phone.data
        db.session.commit()
        return self.__record.id

    def archive(self, biz):
        self.__record = self.get(biz, obj=True)
        self.__record.archived = 1
        db.session.commit()
        return self.__record.bizId

    def delete(self, biz, doc=None):
        if not doc == None:
            self.__record = self.get(biz, doc)
            db.session.delete(self.__record)
            db.session.commit()
            return doc
        self.__record = self.get(biz)
        return biz

The following is from my views.py and both routes reference and use the RecordManager().

@app.route('/manager', methods=["GET", "POST"])
@login_required
@center_required
def biz_manage():
    records = RecordManager()
    data = get_user_data()
    form = BusinessForm()
    form.type.choices = [(c.id, c.name) for c in BizType.query.order_by('id')]
    if form.validate_on_submit() and records.store(form):
        if form.id.data == 'new':
            return redirect(url_for('doc_manage', record=records.id))
        flash('Record has been successfully updated.', 'info')
        return redirect(url_for('biz_manage'))
    data['biz_list'] = records.all()
    return render_template('manager.html', data=data, form=form)

@app.route('/manager/record/<record>', methods=["GET", "POST"])
@login_required
@center_required
def doc_manage(record):
    records = RecordManager()
    data = get_user_data()
    form = DocumentForm()
    form.type.choices = [(c.id, c.name) for c in DocType.query.order_by('id')]
    if form.validate_on_submit() and records.store(form):
        flash('Document has been successfully updated.', 'info')
        return redirect(url_for('doc_manage', record=record))
    data['record'] = records.get(record)
    data['doc_list'] = records.list()
    data['expire'] = doc_expire(data['record'].documents, 30, 1)
    return render_template('record.html', data=data, form=form)

Lastly, this snippet is from my models.py and represents the SQLAlchemy models.

class Center(db.Model):
    id = db.Column(MEDIUMINT(8, unsigned=True), primary_key=True,
                   autoincrement=False)
    phone = db.Column(VARCHAR(10), nullable=False)
    location = db.Column(VARCHAR(255), nullable=False)
    businesses = db.relationship('CenterBusiness', lazy='dynamic')
    employees = db.relationship('CenterEmployee', lazy='dynamic')

class Business(db.Model):
    id = db.Column(MEDIUMINT(8, unsigned=True), primary_key=True,
                   autoincrement=True)
    typId = db.Column(TINYINT(2, unsigned=True),
                      db.ForeignKey('biz_type.id',
                                    onupdate='RESTRICT',
                                    ondelete='RESTRICT'),
                      nullable=False)
    type = db.relationship('BizType', backref='businesses',
                           lazy='subquery')
    name = db.Column(VARCHAR(255), nullable=False)
    contact = db.Column(VARCHAR(255), nullable=False)
    phone = db.Column(VARCHAR(10), nullable=False)
    documents = db.relationship('Document', backref='business',
                                lazy='dynamic')

class CenterBusiness(db.Model):
    cenId = db.Column(MEDIUMINT(8, unsigned=True),
                      db.ForeignKey('center.id',
                                    onupdate='RESTRICT',
                                    ondelete='RESTRICT'),
                      primary_key=True)
    bizId = db.Column(MEDIUMINT(8, unsigned=True),
                      db.ForeignKey('business.id',
                                    onupdate='RESTRICT',
                                    ondelete='RESTRICT'),
                      primary_key=True)
    info = db.relationship('Business', backref='centers',
                           lazy='joined')
    archived = db.Column(TINYINT(1, unsigned=True), nullable=False,
                         server_default='0')

class Document(db.Model):
    id = db.Column(MEDIUMINT(8, unsigned=True), primary_key=True,
                   autoincrement=True)
    typId = db.Column(TINYINT(2, unsigned=True),
                      db.ForeignKey('doc_type.id',
                                    onupdate='RESTRICT',
                                    ondelete='RESTRICT'),
                      nullable=False)
    type = db.relationship('DocType', backref='documents',
                           lazy='subquery')
    bizId = db.Column(MEDIUMINT(8, unsigned=True),
                      db.ForeignKey('business.id',
                                    onupdate='RESTRICT',
                                    ondelete='RESTRICT'),
                      nullable=False)
    store = db.Column(VARCHAR(255), nullable=True)
    expiry = db.Column(db.DateTime, nullable=False)

Answer 1

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:

• 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 Font instance. 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 do font.sizeInPixels = 12, then you know too much about the implementation. The authors cannot refactor the class to use sizeInPoints anymore, without breaking all the code that accesses the internal data directly. If you enforce using only use methods, for example font.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. loadRatesTable will 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 be is not None
  • Likewise, instead of doc == None, should be doc is None
• Instead of if obj: return self.__business, break the line after the :