Simplify small pieces of code with Python/Django [closed]

I'm trying to make a code review for an external developer on my project. His code could be improved and I need your advices in order to know if my overwritten parts are better or not.

Part 1

# INITIAL

if 'error_message' in result_import:
result_import['error_message'] = "The file could not be uploaded. \n" + result_import['error_message']
context['error_message'] = result_import['error_message']
if 'success_message' in result_import:
context['success_message'] = result_import['success_message']
if 'warning_message' in result_import:
context['warning_message'] = result_import['warning_message']

# MODIFIED

if 'error_message' in result_import:
messages.error(request, mark_safe(f"The file could not be uploaded : <br /> {result_import['error_message']}"))
if 'success_message' in result_import:
messages.success(request, result_import['success_message'])
if 'warning_message' in result_import:
messages.warning(request, result_import['warning_message'])


I replaced his context by a django module used at different places in my project django.contrib.messages which is, to my mind, a better way to display messages.

Part 2

# INITIAL

@staticmethod
def get_product_type(value):
try:
if value.text == 'F':
return 'Final'
elif value.text == 'U':
return 'Upstream'
else:
return ''
except AttributeError:
return ''

# MODIFIED

@staticmethod
def get_product_type(value):
""" Docstring """
if value.text == 'F':
return 'Final'
elif value.text == 'U':
return 'Upstream'
else:
return ''


I added docstrings which need to be completed, and from my point of view, else and AttributeError are redondant no ?

Part 3

# INITIAL

@staticmethod
manufacturer_name = ''
try:
if field_relation != '':
manufacturer_name = DataListManufacturerName.objects.get(name=field_relation)
if field_traduction != '' and field_relation == '':
except ObjectDoesNotExist as e:
if field_relation == '' and field_traduction == '':
manufacturer_name = ''
pass

if manufacturer_name:
return manufacturer_name.name
else:
return ''

# MODIFIED

@staticmethod
""" Docstring """
manufacturer_name = ''
try:
if field_relation != '':
manufacturer_name = DataListManufacturerName.objects.get(name=field_relation)
if field_traduction != '' and field_relation == '':
except ObjectDoesNotExist:
if field_relation == '' and field_traduction == '':
manufacturer_name = ''
pass

return manufacturer_name.name if manufacturer_name else ''


Docstrings missing, rewrite the last if statement in order to shorten the code. Possibility to simplify more ?

Part 4

# INITIAL

try:
pheur_name = TranscodedDataEuropeanPharmacopoeiaProductName.objects.get(
name=field_relation if field_relation != '' else field_traduction).data_eppn if not pheur_transcoded_data else pheur_transcoded_data
except ObjectDoesNotExist:
if field_relation == '' and field_traduction == '':
pheur_name = ''
pass
else:
pheur_name = ''

if pheur_name:
return pheur_name.name
else:
return ''

# MODIFIED

try:
if not pheur_transcoded_data:
pheur_name = TranscodedDataEuropeanPharmacopoeiaProductName.objects.get(name=field_relation if field_relation != '' else field_traduction).data_eppn
else:
pheur_name = pheur_transcoded_data
except ObjectDoesNotExist:
pheur_name = ''

return pheur_name.name if pheur_name else ''


Rewrite the try/except part and short the code

Part 5

# INITIAL

except ValidationError as e:
raise e
except IntegrityError as e:
raise e
pass

# MODIFIED

except (ValidationError, IntegrityError) as e:
raise e


I'm not sure if this operation will work if I get a ValidationError or IntegrityError

I'm not an expert in Python/Django, but I need advices.

Thank you very much

• It's rather hard to get a big picture from such small snippets. There is a lot of guesswork as to how these can be used or what they perform. Can you, at least, describe what each snippet is doing and how they will be called (possibly using code). – 301_Moved_Permanently May 23 '19 at 16:05

Part 1: Can't comment it properly, I am not Django user. But looks pretty good.

Part 2: I think it is better to retain try-except block. If a value will not have text parameter, you will get an error. If you don't want try-except block, you should add hasattr check:

if hasattr(value, 'text'):
# DO THE MAGIC!


or:

if not hasattr(value, 'text'):
# WE HAVE PROBLEMS, MY LORD!


About the last else. You can delete it and have this code:

def get_product_type(value):
...
elif value.text == 'U':
return 'Upstream'
return ''


def get_product_type(value):
...
elif value.text == 'U':
return 'Upstream'
else:
return ''


But I don't think it really does matter.

Part 3:

1.

if field_relation != '':
...
if field_traduction != '' and field_relation == '':
...


You don't need the second statement if you will write elif instead of if:

if field_relation != '':
...
...

1. It is a bad idea to set manufacturer_name = '' to string and then change it to class instance. When another programmer will read your code, they can think that it is (and will be) a string because the variable name is fit to this mention. So I suggest you to use manufacturer_name = None in this case.

3.

        if field_relation == '' and field_traduction == '':
manufacturer_name = ''
pass


pass is redundant. You already has a code inside if. Moreover, you can return '' in except so you will not need the last if-statement (you can return the good value in the end of the try or after the try-except block).

1. field_relation == '' and field_relation != '' can be shortened to not field_relation and field_relation

The same for Part 4. Your code can be shortened to:

try:
if pheur_transcoded_data:
return pheur_transcoded_data.name
else:
return Transcodedblah-blah-blahName.objects.get(blah-blah-blah).data_eppn.name
except ObjectDoesNotExist:
return ''


Part 5 is ok.

• Very nice explanation ! It's really good for my developer culture to know this kind of things. From my point of view, this kind of things let to get a maintainable and more readable code ! So thank you ! – Essex May 23 '19 at 15:05
• How is part 5 ok? For all we know it should be deleted. Or special circumstances may make it a good enough improvement, but we have too much of a limited knowledge to be able to answer… – 301_Moved_Permanently May 24 '19 at 12:01