0
\$\begingroup\$

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
def _get_manufacturer_name(field_relation, field_traduction):
    manufacturer_name = ''
    try:
        if field_relation != '':
            manufacturer_name = DataListManufacturerName.objects.get(name=field_relation)
        if field_traduction != '' and field_relation == '':
            manufacturer_name = TranscodedDataListManufacturerName.objects.get(name=field_traduction).data_lmn
    except ObjectDoesNotExist as e:
        if field_relation == '' and field_traduction == '':
            manufacturer_name = ''
            pass

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

# MODIFIED

@staticmethod
def _get_manufacturer_name(field_relation, field_traduction):
    """ Docstring """
    manufacturer_name = ''
    try:
        if field_relation != '':
            manufacturer_name = DataListManufacturerName.objects.get(name=field_relation)
        if field_traduction != '' and field_relation == '':
            manufacturer_name = TranscodedDataListManufacturerName.objects.get(name=field_traduction).data_lmn
    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

\$\endgroup\$

closed as off-topic by Mathias Ettinger, yuri, 200_success, pacmaninbw, esote May 24 at 3:39

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Mathias Ettinger, yuri, 200_success, pacmaninbw, esote
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 2
    \$\begingroup\$ 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). \$\endgroup\$ – Mathias Ettinger May 23 at 16:05
2
\$\begingroup\$

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 ''

instead of yours:

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 != '':
...
elif field_traduction != '':
...
  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.

\$\endgroup\$
  • \$\begingroup\$ 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 ! \$\endgroup\$ – Essex May 23 at 15:05
  • \$\begingroup\$ 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… \$\endgroup\$ – Mathias Ettinger May 24 at 12:01

Not the answer you're looking for? Browse other questions tagged or ask your own question.