Как сделать этот код более компактным и читаемым


Мой ниже код работает нормально, но я хочу сделать это более подходящие для Python. Кроме того, я хочу избавиться от вложенных IFS и сделать код более компактным и читаемым. Какие-либо предложения? Здесь project_id и status_change_to string объекты. Этот код обновляет статус сертификата.

    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


140
-2
задан 21 февраля 2018 в 07:02 Источник Поделиться
Комментарии
1 ответ

Просто это как я читал:

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

Это, кажется, очень близки к

if status_change:

так что, если есть веская причина (хорошая причина) писать многословные и повторяющиеся версии, вы должны использовать более короткий вариант. Очевидно, это будет иметь различное поведение, когда status_change == 0 например, но я думаю, что можно предположить из контекста, что status_change это предназначается, чтобы быть либо строкой, либо (как частный случай) None.


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

Это странный отступ, и я предпочитаю не использовать обратные слеши в мой собственный код. Так что я бы написал что-то вроде

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

(Запятую в конце каждой строки-это просто привычка у меня сформирована за счет длительного воздействия нечистых git diff выход, предположительно, она будет безвредна для падения последней запятой в этом случае, если вы уверены, что не будет больше аргументов добавил. Но тогда, в программной инженерии, мы всегда "конечно", не так ли...?)


else:
pass

Это, очевидно, глупо.


if cert_obj_json != "":

То же, что и выше: если вы уже знаете тип объекта, а лишь хотите убедиться, что он не пуст, вы можете использовать его truthiness. Кроме того, у вас есть все функции организма при этом if. Мы можем исправить это как можно скорее вернуться:

    if not cert_obj_json:
return

Но на самом деле, я думаю, что лучше было бы утверждать в этом случае. Потому что если cert_obj_json является недействительным, что пользователь делает вызов этой функции вообще?

    assert cert_obj_json, "I need a JSON object"

И тогда мы понимаем, что мы можем просто снять утверждение, и пусть json.loads не шумно; абонент будет получать те же общие симптомы (исключение) и придется пойти, чтобы исправить свои ошибки код, в любом случае. Поэтому мы полностью исключили эту строку кода!


            )
# Update provider details
if service_obj is not None:

Ваш отступ здесь шаткий.
Это напомнило мне, за советы по стилю, вы должны запустить все это дело через Линтер, таких как flake8. В Линтер будет ловить все ваши странные отступы и такие, и даже иногда сказать вам, как это исправить.


Собираем все вместе, у нас есть это:

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,
)

У меня еще есть два хитрых комментариев:


  • Эти названия метода супер длинные, поэтому у вас возникли проблемы с длиной строки. Вы контролируете их? Не могли бы вы переименовать например self.service_storage.update_provider_details для self.storage.set_provider?

  • Линии

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

    очень отрывочно, в том, что он мутирует: он принимает значение подобъекта cert_details['Fastly'] и назначив ему новое значение, которое конечно оказывает влияние на все программы, которые могли бы смотреть на большой объект cert_details. Вы должны попробовать, чтобы избежать мутации такого рода в ваших программах. Но в данном случае я не вижу каких-либо легко исправить, поэтому я просто упомяну об этом, а не фиксируя его для вас.



Ох, и перечитывая мои очищен код, я вижу, что если status_change_to это falsey, то мы только собираемся, чтобы загрузить JSON и затем немедленно вернуться сделав ничего важного. Так что я подозреваю, мы могли бы применить ту же логику сверху: заменить if С раннего возврата, затем замените досрочное возвращение с assertтогда устранить assert как излишнее.

Результат:

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,
)

Для получения дополнительной информации о рефакторинга, я настоятельно рекомендую книгу "элементы программирования стиль", по Керниган и п. плаугер.

6
ответ дан 21 февраля 2018 в 08:02 Источник Поделиться