Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm new to Python and GAE, but years of procedural programming. I have this code, but I know that there must be some better solution to avoid retyping code.

The first and last section of code are identical in both GET and POST methods, so I suppose there must be another way to share the identical code.

# This class manage the user account
class MyAccount(webapp2.RequestHandler):
        def get(self):
                """ Shows actual user account """

                # Get actual user
                user = users.get_current_user()

                if user:
                        # Get the actual user data
                        data = Person.gql("WHERE username = :nick",
                                                            nick=user.nickname()).get()

                        # Test if we found the user
                        if data:
                                templateValues['data'] = data
                                template = jinja_environment.get_template('myAccount.htm')
                        else:
                                templateValues['custom_msg'] = "The logged user is not available."
                                template = jinja_environment.get_template('customMessage.htm')

                else:
                        templateValues['custom_msg'] = "You are not logged in. Please, login to access to your account"
                        template = jinja_environment.get_template('customMessage.htm')

                # Render the page
                self.response.out.write(template.render(templateValues))

        def post(self):
                """ Saves user updated data """

                # Get actual user
                user = users.get_current_user()

                if user:
                        # Get the actual user data
                        person = Person.gql("WHERE username = :nick",
                                                            nick=user.nickname()).get()

                        # Test if we found the user
                        if person:
                                person.username = self.request.get('username')
                                person.password = self.request.get('password')
                                if person.email: self.request.get('email')
                                person.name = self.request.get('name')
                                person.lastname = self.request.get('lastname')
                                person.idnumber = self.request.get('idnumber')

                                templateValues['data'] = person
                                template = jinja_environment.get_template('myAccount.htm')

                        else:
                                templateValues['custom_msg'] = "The logged user is not available."
                                template = jinja_environment.get_template('customMessage.htm')

                else:
                        templateValues['custom_msg'] = "You are not logged in. Please, login to access to your account."
                        template = jinja_environment.get_template('customMessage.htm')

                # Render the page
                self.response.out.write(template.render(templateValues))
share|improve this question

2 Answers 2

up vote 1 down vote accepted

This line looks suspicious:

            if person.email: self.request.get('email')

I assume it should be:

            person.email = self.request.get('email')

Your code seem to be using excessive indentation. The python standard is four space per level.

You are using templateValues but don't seem to define it anywhere.

To refactor this, I'd start by removing the section of that that's different between the two methods into their own function:

# This class manage the user account
class MyAccount(webapp2.RequestHandler):

    def show_user(self, user):
        # Get the actual user data
        data = Person.gql("WHERE username = :nick",
                                            nick=user.nickname()).get()
        # Test if we found the user
        if data:
            templateValues['data'] = data
            template = jinja_environment.get_template('myAccount.htm')
        else:
            templateValues['custom_msg'] = "The logged user is not available."
            template = jinja_environment.get_template('customMessage.htm')

        return template, templateValues



    def get(self):
        """ Shows actual user account """

        # Get actual user
        user = users.get_current_user()
        if user:
            template, templateValues = self.show_user(user)
        else:
            templateValues['custom_msg'] = "You are not logged in. Please, login to access to your account"
            template = jinja_environment.get_template('customMessage.htm')

        # Render the page
        self.response.out.write(template.render(templateValues))

    def update_user(self, user):
        # Get the actual user data
        person = Person.gql("WHERE username = :nick",
                nick=user.nickname()).get()
        # Test if we found the user
        if person:
            person.username = self.request.get('username')
            person.password = self.request.get('password')
            person.email = self.request.get('email')
            person.name = self.request.get('name')
            person.lastname = self.request.get('lastname')
            person.idnumber = self.request.get('idnumber')

            templateValues['data'] = person
            template = jinja_environment.get_template('myAccount.htm')
        else:
            templateValues['custom_msg'] = "The logged user is not available."
            template = jinja_environment.get_template('customMessage.htm')

        return template, templateValues


    def post(self):
        """ Saves user updated data """
        # Get actual user
        user = users.get_current_user()
        if user:
            template, templateValues = self.update_user(user)
        else:
            templateValues['custom_msg'] = "You are not logged in. Please, login to access to your account."
            template = jinja_environment.get_template('customMessage.htm')
        # Render the page
        self.response.out.write(template.render(templateValues))

The only thing different between post and get is the function they call. We'll just pass the function as a parameter to a new function.

    def request(self, handler):
        """ Shows actual user account """

        # Get actual user
        user = users.get_current_user()
        if user:
            template, templateValues = handler(user)
        else:
            templateValues['custom_msg'] = "You are not logged in. Please, login to access to your account"
            template = jinja_environment.get_template('customMessage.htm')

        # Render the page
        self.response.out.write(template.render(templateValues))

    def get(self):
        return self.request(self.show_user)

    def post(self):
        return self.request(self.update_user)

The current code repeats calls to get_template a lot. So we'll refactor it so that it only happens once. We'll just store the name of the template in a variable and load it just before we render it.

def show_user(self, user):
    # Get the actual user data
    data = Person.gql("WHERE username = :nick",
                                        nick=user.nickname()).get()
    # Test if we found the user
    if data:
        templateValues['data'] = data
        template = 'myAccount.htm'
    else:
        templateValues['custom_msg'] = "The logged user is not available."
        template = 'customMessage.htm'

    return template, templateValues



def request(self, handler):
    """ Shows actual user account """

    # Get actual user
    user = users.get_current_user()
    if user:
        template, templateValues = handler(user)
    else:
        templateValues['custom_msg'] = "You are not logged in. Please, login to access to your account"
        template = 'customMessage.htm'

    # Render the page
    template = jinja_environment.get_template(template)
    self.response.out.write(template.render(templateValues))

But we can clean up our actual work functions a little more:

def show_user(self, user):
    # Get the actual user data
    data = Person.gql("WHERE username = :nick",
                                        nick=user.nickname()).get()
    # Test if we found the user
    if data:
        return 'myAccount.htm', {
                'data' : data
        }
    else:
        return 'custom_msg.htm', {
                'custom_msg' : "The logged user is not available."
        }
share|improve this answer
    
Yeah, thanks. I love python everyday more and more. It's flexibility has no limits!! Great trick the function as parameter to a function –  Eagle Jan 30 '13 at 16:52
    
By the way, I pasted the code in my text editor, which made this strange indentation (double tab, instead tab), but is 4 spaces normally. And you are right in the other 2 supositions. –  Eagle Jan 30 '13 at 17:04

A little more clean up (sorry, I can't code on the answer link):

Get inside request the common part for show_user and update_user (which only differs on the name of the var, data or person). This is the final code. Thank you very much.

# This class manage the user account
class MyAccount(webapp2.RequestHandler):

    def show_user(self, person):
        templateValues = {}
        templateValues['data'] = person
        template = 'myAccount.htm'

        return template, templateValues

    def update_user(self, person):
        person.username = self.request.get('username')
        person.password = self.request.get('password')
        person.email = self.request.get('email')
        person.name = self.request.get('name')
        person.lastname = self.request.get('lastname')
        person.idnumber = self.request.get('idnumber')

        templateValues = {}
        templateValues['data'] = person
        template = 'myAccount.htm'

        return template, templateValues

    def requested(self, handler):
        """ Shows actual user account """

        # Get actual user
        user = users.get_current_user()
        if user:
            # Get the actual user data
            person = Person.gql("WHERE username = :nick",
                            nick=user.nickname()).get()
            # Test if we found the user
            if person:
                template, templateValues = handler(person)
            else:
                templateValues['custom_msg'] = "The logged user is not available."
                template = 'customMessage.htm'
        else:
            templateValues['custom_msg'] = "You are not logged in. Please, login to access to your account"
            template = 'customMessage.htm'

        # Render the page
        template = jinja_environment.get_template(template)
        self.response.out.write(template.render(templateValues))

    def get(self):
        return self.requested(self.show_user)

    def post(self):
        return self.requested(self.update_user)
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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