0

I have the following function in Python:

def _extract_grp_entitlements(self,saml_authentication_attributes,groups):
    result = []
    input_length = len(saml_authentication_attributes[groups])
    if input_length == 0:
        log.error(self.empty_entitlements_message)
        raise RuntimeError(self.empty_entitlements_message)
    if input_length == 1:
        result = [t.strip() for t in saml_authentication_attributes[groups][0].split(',')]
    elif input_length:
        result = saml_authentication_attributes[groups]
    return result

Are there any benefits/drawbacks (aside from the logical control flow) - speed, memory, etc. - for replacing the elif there with the else clause?

Would this be preferable:

    def _extract_grp_entitlements(self,saml_authentication_attributes,groups):

        input_length = len(saml_authentication_attributes[groups])

        if input_length == 0:
            log.error(self.empty_entitlements_message)
            raise RuntimeError(self.empty_entitlements_message)


        return [t.strip() for t in saml_authentication_attributes[groups][0].split(',')] \
            if len(saml_authentication_attributes[groups]) == 1\
            else saml_authentication_attributes[groups]
Simeon Leyzerzon
  • 18,658
  • 9
  • 54
  • 82
  • 1
    Changing the `elif` to an `else` would make your code clearer. The condition in `elif input_length` must be true at that point in the code. – khelwood Oct 13 '18 at 20:02
  • 2
    Based on common sense, I would think that `else` would be faster than `elif` as there are no condition checks. – Bala Oct 13 '18 at 20:03
  • 2
    Faster doesn't matter here. The speed of checking if a list is empty or not is microscopic. Go for the clearest expression of the idea. – Ned Batchelder Oct 13 '18 at 20:10
  • 1
    Your second version is definitely not preferable. Putting an `if...then` statement into a 'one-liner' that is so long it has to break across 3 lines, just loses clarity with no or negligible performance benefit. – Stuart Oct 13 '18 at 22:31
  • And performance is not the concern here....! Make the code readable. – Ned Batchelder Oct 13 '18 at 22:56

3 Answers3

3

An else would be clearer. Your elif will always run, so there's no point having a condition on it.

Ned Batchelder
  • 364,293
  • 75
  • 561
  • 662
0

Your first function is already readable enough, and performance is unlikely to be a concern. For maximum readability in a short function, I would write it like this:

def _extract_grp_entitlements(self,saml_authentication_attributes,groups):
    inp = saml_authentication_attributes[groups]
    if inp:    
        if len(inp) == 1:
            return [t.strip() for t in inp[0].split(',')]
        else:
            return inp
    else:
        log.error(self.empty_entitlements_message)
        raise RuntimeError(self.empty_entitlements_message)

It seems to me this makes the flow obvious from a quick glance. The elses are both unnecessary (because the function will have returned already if the condition was true) and serve only to make it more explicit. Some prefer to omit else after return.

For longer functions it can become a pain to have all of the main logic nested like that, and it will no longer be clear what condition the trailing else at the end referred to, so it's more convenient to handle basic problems with the parameters at the top.

def _extract_grp_entitlements(self,saml_authentication_attributes,groups):
    inp = saml_authentication_attributes[groups]

    # get the no input error out of the way
    if not inp:
        log.error(self.empty_entitlements_message)
        raise RuntimeError(self.empty_entitlements_message)

    # now do everything else (no need for else)
    if len(inp) == 1:
    # etc.
Stuart
  • 9,597
  • 1
  • 21
  • 30
-1

I managed to shorten the second version of the function to make it both readable and succinct:

def _extract_grp_entitlements(self,groups):

    groups_length = len(groups)

    if groups_length == 0:
        log.error(self.empty_entitlements_message)
        raise RuntimeError(self.empty_entitlements_message)

    return [t.strip() for t in groups[0].split(',')] \
        if groups_length == 1\
        else groups
Simeon Leyzerzon
  • 18,658
  • 9
  • 54
  • 82