1
\$\begingroup\$

I am currently using PyMongo + Flask for building a simple blogging application. I am not using any kind of ODM, instead I decided to use PyMongo directly. I need to know how to improve my code since learning is my main motivation for building this application.

This portion of code is implemented in a view controller called post.py and it is used to edit a post:

@posts.route('/<_id>/edit', methods=['GET', 'POST'])
def edit(_id):
    post = Post()
    post.get(_id)
    if request.method == 'POST':
        post.title = request.form.get('title')
        post.tags = [tag.strip() for tag in 
                     request.form.get('tags').split(',')]
        post.body = request.form.get('body')
        post.update()
    return render_template('posts_edit.html', post=post)

The Post model looks something like this:

class Post:
    def __init__(self, _id=None, title=None, status=None, tags=None, 
                 body=None, date=None, author=None):
        self._id = _id
        self.title = title
        self.status = status
        self.tags = tags
        self.body = body
        self.date = date
        self.author = author

    def save(self):
        mongo.db.posts.insert_one({
            "_id": ObjectId(),
            "title": self.title,
            "status": self.status,
            "tags": self.tags,
            "body": self.body,
            "date": datetime.utcnow(),
            "author": self.author
        })

    def update(self):
        mongo.db.posts.update({
            '_id': self._id
        },
            {
                "title": self.title,
                "status": self.status,
                "tags": self.tags,
                "body": self.body,
                "date": self.date,
                "author": self.author
        })

    def get(self, _id):
        post = mongo.db.posts.find_one({'_id': ObjectId(_id)})
        if post:
            self.__init__(**post)
            return self
        return None

    @classmethod
    def get_all(cls):
        return mongo.db.posts.find()

Is this the right direction?

\$\endgroup\$
  • 1
    \$\begingroup\$ "Is this the right direction?" Does it work properly? \$\endgroup\$ – πάντα ῥεῖ Apr 20 '17 at 19:33
  • \$\begingroup\$ Yes, but is this bad code? I am worried about the quality \$\endgroup\$ – Arturo Reyes Jr. Apr 20 '17 at 19:39
0
\$\begingroup\$

For the route I'd probably use fewer temporary variables, not that it hurts a lot, but it can be more compact. Additionally if Post implemented some sort of dict-like update function it could be even more compact, but that's splitting hairs.

@posts.route('/<_id>/edit', methods=['GET', 'POST'])
def edit(_id):
    post = Post()
    post.get(_id)
    if request.method == 'POST':
        post.title = request.form.get('title')
        post.tags = [tag.strip()
                     for tag in request.form.get('tags').split(',')]
        post.body = request.form.get('body')
        post.update()
    return render_template('posts_edit.html', post=post)

With the latter point I mean something like this (implemention left as an exercise for the reader):

@posts.route('/<_id>/edit', methods=['GET', 'POST'])
def edit(_id):
    post = Post()
    post.get(_id)
    if request.method == 'POST':
        post.update({
            "title": request.form.get('title'),
            "tags": [tag.strip()
                    for tag in request.form.get('tags').split(',')]
            "body": request.form.get('body')
        })
    return render_template('posts_edit.html', post=post)

For the other file I'd suggest looking at the usual 80 character limit, meaning wrapping the longer lines a bit.

  • In save the date and _id attributes of the object aren't being used - that looks inconsistent, either self.date and self._id should be updated too, or neither, right? That should probably also be documented clearly.

  • With get and the __init__ call, AFAIK that's probably safe, but coming from any background where constructors are, well, that, this looks fishy. Consider making that a class method too:

    @classmethod
    def get(cls, _id):
        return cls(**mongo.db.posts.find_one({'_id': ObjectId(_id)}))
    
  • get_all doesn't use it's argument, so it's actually more of a staticmethod right now. If you're no going to use inheritance make it one then. If you are, possibly make move mongo.db.posts into a class variable so that you could override it for subclasses and the methods would keep on working with the correct tables.

\$\endgroup\$
  • 1
    \$\begingroup\$ self._id and self.date should not be updated, but you are right about documenting it. Your second point is absolutely valid. Lastly, I am indeed planning on using inheritance. Thanks a lot! \$\endgroup\$ – Arturo Reyes Jr. Apr 20 '17 at 21:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.