Which of these two coding-styles is better? Both are compliant with PEP8 [closed]

Question

Closed. This question is opinion-based. It is not currently accepting answers.

---

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post.

Closed 8 years ago.

Improve this question

I'm trying to be quite strict about PEP8 coding style, but this question has not been answered for me. These are two versions of the same code, one is using temporary variables of which each is used only once, the other version doesn't use temporary variables and looks more like what would be common in functional languages.

For me the functional one looks prettier, but I'm not sure if there is any guideline about how many functions I should chain together.

Version with temporary variables:

   class EmailConfirmation():

       @receiver(email_confirmed)
       def confirmed(sender, **kwargs):
           email = kwargs['email_address'].email
           keystone_id = User.objects.get_by_natural_key(email).keystone_id
           client = Client(token=settings.KEYSTONE_TOKEN,
                           endpoint=settings.KEYSTONE_URL)
           client.users.update(keystone_id, enabled=True)

Version without temporary variables:

  class EmailConfirmation():

      @receiver(email_confirmed)
      def confirmed(sender, **kwargs):
          Client(
              token=settings.KEYSTONE_TOKEN,
              endpoint=settings.KEYSTONE_URL
          ).users.update(
              User.objects.get_by_natural_key(
                  kwargs['email_address'].email
              ).keystone_id, enabled=True
          )

Is there any guideline that defines which of these two versions is recommended, or are both ok?

Answer 1

The first version should be preferred. This is true, not just for Python, but for other languages like C++/Java because its prone to error and per the Zen of Python Errors should never pass silently., which would be the case in your second version.

The Reason:

Consider, the instantiate of Client fails, which results in returning None. Chaining the same object without checking for a success or proper error handling would result in errors unrelated to the actual problem AttributeError: 'NoneType' object has no attribute 'users'.

So it is always better, to avoid chaining objects, as that would cause errors to pass silently.

Example

Consider the following modification to your first version

class EmailConfirmation():
   @receiver(email_confirmed)
   def confirmed(sender, **kwargs):
       email = kwargs['email_address'].email
       keystone_id = User.objects.get_by_natural_key(email).keystone_id
       try:
           client = Client(token=settings.KEYSTONE_TOKEN,
                           endpoint=settings.KEYSTONE_URL)
       except CustomClientError as e:
           # Your Error Handling Code goes here
           raise CustomClientError("Your Error Message")
       else:
           if client.users:
               client.users.update(keystone_id, enabled=True)
           else:
               raise CustomEmailError("Your Error Message")
       finally:
           # Cleanup code goes here

Answer 2

PEP8 doesn't dictate whether or not to use temporary variables.

To my eyes, though, the second version does not look very Pythonic: most Python code avoids chaining methods like that. I might even store the User rather than the keystone_id:

user = User.objects.get_by_natural_key(email)
client = Client(token=settings.KEYSTONE_TOKEN,
                endpoint=settings.KEYSTONE_URL)
client.users.update(user.keystone_id, enabled=True)