4
\$\begingroup\$

I want to differentiate between comments I make for

  • (1) myself,
  • (2) team members,
  • (3) on business rules,
  • (4) on watchout notes for hardcoded constants,
  • (5) on ideas for improvements, and
  • (6) on general flow of the code.

Right now all my comments look the same.

I'm open to any other tools. I don't know how bookmarking works, or if I really should be adding debug points as a "TODO" (the auto "TODO" font is weird and intrusive), but I'm open to anything.

I use PyCharm.

As for what this code accomplishes, it takes in an input of data on students who tested for a Science exam and filters out the students with errors. That means errors in scoring that a school needs to resolve. Therefore, it gives back a highly filtered list of students (200 rows) from a much larger (40,000 rows) dataset.

import pandas as pd
import numpy as np
import xlrd
import openpyxl
import win32com.client
import paths
import utils as u

'''
Extraction
'''
# (4) SXRX row starts at row 5
# (4) SXRX has 19 rows at its footer
SXRX_df = pd.read_excel(paths.corr_outputs_path, sheet_name='SXRX', header=4, skipfooter=19)
# (4) drop first row because it's blank
SXRX_df = SXRX_df[1:]

# (5) better to do writer?
corr_outputs_df = pd.read_excel(paths.corr_outputs_path, sheet_name='Correct Outputs')

'''
Operations
'''
def reason_incomplete(school_follow_up, site_follow_up, errors):
    """
    We expect mainly P.1 Not Scanned. Teacher errors, while rare, will still exist (especially in Charter schools)

    :param school_follow_up: RCSD school follow up
    :param site_follow_up: RCSD site follow up
    :param errors: RCSD errors, can come in the format of 046/57, sometimes none
    :return: text according to business rules
    """

    # (2) explicitly cast errors as string, because sometimes it will come in as a float and the "in errors" syntax will
    # not work
    errors = str(errors)
    if school_follow_up == 'P.1 Not Scanned':
        return 'P.1 Not Scanned'
    elif site_follow_up == 'FOLLOW UP':
        return 'P.2 Not Scanned'
    elif '40' in errors:
        return 'Abs + Response'
    elif '41' in errors:
        return 'Multiples in Student Answers'
    elif '47' in errors:
        return 'Omits in Teacher Answers'
    elif '46' in errors:
        return 'Multiples in Teacher Answers'
    else:
        return None

df = pd.DataFrame()

# (1) genericize later

df['Last Name'] = SXRX_df['Last Name']

df['First Name'] = SXRX_df['First Name']

df['Student ID'] = SXRX_df['Student Id']

df['School DBN'] = SXRX_df['School DBN']

df['Ofc Cls'] = SXRX_df['Cls']

df['Section'] = SXRX_df['Section']

df['Final Score'] = SXRX_df['Score.1']

df['Reason Incomplete'] = np.vectorize(reason_incomplete)(SXRX_df['Follow Up'],
                                                          SXRX_df['Follow Up.1'],
                                                          SXRX_df['Errors'])

df['Exam'] = 'SXRX'

# (6) filter out Reason Incomplete Nones

# (6) filter out Final Score INV and MIS

# (6) reorder columns
df = df.reindex(columns=corr_outputs_df.columns)
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. In order to give you proper advice, we must first understand the code ourselves. Please explain what this code accomplishes, ideally with example inputs and outputs. See How to Ask. \$\endgroup\$ – 200_success Aug 8 at 20:47
  • \$\begingroup\$ Are the comments used as temporary TODO's, as a discussion starter, as static information about some code? \$\endgroup\$ – dfhwze Aug 9 at 8:40
2
\$\begingroup\$

Meta discussions vs Code comments

You should distinguish comments meant for discussion with team members and code comments. Since you work in a team, I take it you have a source control system. For instance, GIT allows to make a Pull Request. This means you put your code up for the team to review. Comments can be made on this pull request, rather than polluting the code. You can even set a policy that all comments need to be 'resolved' before the code is allowed to be merged back into the master branch.


Review

A note on your choice of tags. You use (1) for self and (2) for team members. What if team members use the same system? Who would be the author of tag (1)? Choose your tags more carefully regarding the fact you are a team.

I want to differentiate between comments I make for

(1) myself, (2) team members, ..

Perhaps it's not such a bad thing that TODO's are intrusive. This gives you an additional motivation to refactor the code and work those TODO's out.

.. the auto "TODO" font is weird and intrusive

\$\endgroup\$
5
\$\begingroup\$

I think the major problem with this current system is this: Imagine you come back 6 month after having written these comments and have not used this commenting system elsewhere since then. What category does a comment like (4) ... belong to? Without looking it up! Now imagine someone else looks at your code and needs to figure out which comments are meant for them.

In other words, the system is not self explanatory.

(Ab)using multi-line strings as comments is also discouraged. You are not even using them as multi-line string anyway.

It would already help if you used words instead of numbers. Maybe this is already enough to cover all your cases:

  • # TODO NAME note to name or # FIXME important bugfix
  • # TODO TEAM note to team members,
  • # just plain comments for business rules
  • # CONSTANT description
  • # NOTE idea for improvement
  • # just plain comments on the flow of the code

At least in my editor, TODO, FIXME and NOTE all get the same special highlight.


Your code itself you should re-organize a bit. Ideally you define all your classes and functions first and put code calling it afterwards. Don't intersperse the two.

The definition of df can also be simplified a bit using extended indexing of pandas dataframes and pandas.DataFrame.rename:

df = SXRX_df[['Last Name', 'First Name', 'Student Id', 'School DBN', 'Cls', 'Section', 
 'Score.1']]
df = df.rename(columns={'Cls': 'Ofc Cls', 'Score.1': 'Final Score'})
df['Exam'] = 'SXRX'
df['Reason Incomplete'] = reason_incomplete(SXRX_df['Follow Up'],
                                            SXRX_df['Follow Up.1'],
                                            SXRX_df['Errors'])

Note that the columns are in the order in which they appeared in the list, so you might not even need the reindex.

The np.vectorize can also already be used as a decorator at the time of function definition and save some real estate in the block above:

@np.vectorize
def reason_incomplete(school_follow_up, site_follow_up, errors):
    ...
\$\endgroup\$

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.