The following script derives schemas from varied file formats. Can I get this reviewed? What would the improvements look like:

    def _null_allowed_in_cols(self):
        for field in self.fields:
            field["type"] = self._field_type_converter(field["type"])
            if type(field["type"]) == list:
                if "null" not in field["type"]:
                    field["type"].insert(0, "null")
                field["type"] = ["null", field["type"]]

    def _field_type_converter(self, field_type) -> Union[str, list]:
        if type(field_type) is dict or type(field_type) is avro.schema.ImmutableDict:
            return field_type["type"]
        elif type(field_type) is list:
            return list(map(lambda x: self._field_type_converter(x), field_type))
            return field_type
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Vogel612
    Aug 24, 2021 at 21:22

1 Answer 1



You can use a generator expression here to return either the first match or None:

    # go from this
    def _col_type(self, column_name):
        for column in self.fields:
            if column["name"] == column_name:
                return column
        return None

   # to this
   def _col_type(self, column_name):
       return next((
        col['name'] for col in self.fields if col['name'] == column_name
       ), None)

Checking types

Use isinstance rather than type(val) == cls:

# instead of this
if type(field["type"]) == list:
    # do something
# or this
if type(val) is dict:
    # do something

# do this instead
if isinstance(field['type'], list):
    # do something

# or
if isinstance(field_type, (dict, avro.schema.ImmutableDict)):
    # do something

When to use lambda vs function

In this statement:

list(map(lambda x: self._field_type_converter(x), field_type))

You don't actually need the lambda:

list(map(self._field_type_converter, field_type))

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.