-2
\$\begingroup\$

My below code is working fine but I want to make it more Pythonic. Furthermore, I want to get rid of nested ifs and make the code more compact and readable. Any suggestions? Here project_id and status_change_to are string objects. This code updates the status of certificate.

    def execute(self, project_id, cert_obj_json, status_change_to):
        if cert_obj_json != "":
            cert_obj = ssl_certificate.load_from_json(
                json.loads(cert_obj_json)
            )
            cert_details = cert_obj.cert_details

            if status_change_to != "" and status_change_to is not None:
                cert_details['Fastly']['extra_info']['status'] = (
                    status_change_to)
                cert_details['Fastly'] = json.dumps(cert_details['Fastly'])
                self.storage_controller.update_certificate(
                    cert_obj.domain_name,
                    cert_obj.cert_type,
                    cert_obj.flavor_id,
                    cert_details
                )

                service_obj = (
                    self.service_storage.
                    get_service_details_by_domain_name(cert_obj.domain_name)
                )
            # Update provider details
                if service_obj is not None:
                    service_obj.provider_details['Fastly'].\
                        domains_certificate_status.\
                        set_domain_certificate_status(cert_obj.domain_name,
                                                  status_change_to)
                    self.service_storage.update_provider_details(
                        project_id,
                        service_obj.service_id,
                        service_obj.provider_details
                    )
            else:
                pass
\$\endgroup\$
  • 4
    \$\begingroup\$ Can you edit the post to explain what this code is supposed to do? What kinds of object are project_id, cert_obj_json, status_change_to etc? \$\endgroup\$ – Gareth Rees Feb 21 '18 at 8:01
  • \$\begingroup\$ @GarethRees project_id is a string of number, status_change_to is a string again and cert_obj_json is a json object.. \$\endgroup\$ – Raman Balyan Feb 21 '18 at 8:04
  • \$\begingroup\$ Welcome to Code Review! This question is incomplete. To help reviewers give you better answers, please edit to add sufficient context to your question. The more you tell us about what your code does, the easier it will be for reviewers to help you. Questions should include a description of what the code does. It appears that this is a fragment of a class definition, but it's not reviewable outside of that context - we can't see what self is! \$\endgroup\$ – Toby Speight Feb 21 '18 at 15:43
6
\$\begingroup\$

Just going through it as I read it:

if status_change_to != "" and status_change_to is not None:

This seems awfully close to

if status_change:

so, unless there is some good reason (good reason) to write the verbose and repetitive version, you should use the shorter version. Obviously it'll have different behavior when status_change == 0 for example, but I think we can assume from context that status_change is meant to be either a string or (as a special case) None.


service_obj.provider_details['Fastly'].\
    domains_certificate_status.\
    set_domain_certificate_status(cert_obj.domain_name,
                              status_change_to)

This is weird indentation, and I prefer not to use backslashes in my own code. So I would write something like

fastly = service_obj.provider_details['Fastly']
fastly.domains_certificate_status.set_domain_certificate_status(
    cert_obj.domain_name,
    status_change_to,
)

(The comma at the end of each line is just a habit I've formed through long exposure to unclean git diff output; presumably it would be harmless to drop the final comma in this case, if you're sure there will never be any more arguments added. But then, in software engineering we're always "sure", aren't we...?)


else:
    pass

This is obviously silly.


if cert_obj_json != "":

Same deal as above: if you already know the expected type of the object, and just want to check that it's not-empty, you can use its truthiness. Also, you've got the entire function body under this if. We can fix that with an early return:

    if not cert_obj_json:
        return

But actually, I would guess that it would be better to assert in this case. Because if cert_obj_json is invalid, what is the user doing calling this function at all?

    assert cert_obj_json, "I need a JSON object"

And then we realize that we could just remove the assertion and let json.loads fail noisily; the caller will get the same general symptom (an exception) and will have to go fix their buggy code either way. So we've completely eliminated this line of code!


            )
        # Update provider details
            if service_obj is not None:

Your indentation here is wonky. Which reminds me, for style tips, you should run the whole thing through a linter such as flake8. The linter will catch all your weird indentation and such, and even sometimes tell you how to fix it.


Putting it all together, we have this:

def execute(self, project_id, cert_obj_json, status_change_to):
    cert_obj = ssl_certificate.load_from_json(
        json.loads(cert_obj_json)
    )
    cert_details = cert_obj.cert_details

    if status_change_to:
        cert_details['Fastly']['extra_info']['status'] = status_change_to
        cert_details['Fastly'] = json.dumps(cert_details['Fastly'])
        self.storage_controller.update_certificate(
            cert_obj.domain_name,
            cert_obj.cert_type,
            cert_obj.flavor_id,
            cert_details,
        )

        service_obj = self.service_storage.get_service_details_by_domain_name(
            cert_obj.domain_name
        )
        if service_obj:
            fastly = service_obj.provider_details['Fastly']
            fastly.domains_certificate_status.set_domain_certificate_status(
                cert_obj.domain_name,
                status_change_to,
            )
            self.service_storage.update_provider_details(
                project_id,
                service_obj.service_id,
                service_obj.provider_details,
            )

I still have two trickier comments:

  • These method names are super long, which is why you're having trouble with line lengths. Do you control them? Could you rename e.g. self.service_storage.update_provider_details to self.storage.set_provider?

  • The line

    cert_details['Fastly'] = json.dumps(cert_details['Fastly'])
    

    is super sketchy, in that it is mutating: it's taking the value of the subobject cert_details['Fastly'] and assigning a new value to it, which of course has an effect on everyone in the program who might be looking at the bigger object cert_details. You should try to avoid mutation of this kind in your programs. But in this case I don't see any easy fix, so I just mention it, rather than fixing it for you.


Oh, and re-reading my cleaned-up code, I see that if status_change_to is falsey, then we're just going to load the JSON and then immediately return having done nothing important. So I suspect we could apply the same logic from above: replace the if with an early return, then replace the early return with an assert, then eliminate the assert as redundant.

Result:

def execute(self, project_id, cert_obj_json, status_change_to):
    assert cert_obj_json
    assert status_change_to
    cert_obj = ssl_certificate.load_from_json(
        json.loads(cert_obj_json)
    )

    cert_details = cert_obj.cert_details
    cert_details['Fastly']['extra_info']['status'] = status_change_to
    cert_details['Fastly'] = json.dumps(cert_details['Fastly'])

    self.storage_controller.update_certificate(
        cert_obj.domain_name,
        cert_obj.cert_type,
        cert_obj.flavor_id,
        cert_details,
    )

    service_obj = self.service_storage.get_service_details_by_domain_name(
        cert_obj.domain_name
    )
    if service_obj is None:
        return

    fastly = service_obj.provider_details['Fastly']
    fastly.domains_certificate_status.set_domain_certificate_status(
        cert_obj.domain_name,
        status_change_to,
    )
    self.service_storage.update_provider_details(
        project_id,
        service_obj.service_id,
        service_obj.provider_details,
    )

For more information on refactorings like this, I highly recommend the book "The Elements of Programming Style", by Kernighan and Plauger.

\$\endgroup\$
  • \$\begingroup\$ thanks for the detailed information..I just didnt get this comment of yours and you havent added this in the final result code of yours: if cert_obj_json != "": Same deal as above: if you already know the expected type of the object, and just want to check that it's not-empty, you can use its truthiness. Also, you've got the entire function body under this if. We can fix that with an early return: if not cert_obj_json: return \$\endgroup\$ – Raman Balyan Feb 21 '18 at 9:07
  • 1
    \$\begingroup\$ @RamanBalyan An empty string evaluates to False, just like a non-empty string evaluates to True, when the string is used in a boolean expression. That's why you can replace if my_string != "" (which just means my_string is not empty) with if my_string (which means my_string is not empty (and also not None)). \$\endgroup\$ – Raimund Krämer Feb 21 '18 at 10:58

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.