wiki:CodeReviewNotes

Toward a Code Review Checklist

Item Ready? Detail
Use Case Clear? ? detail
Automated Feature Tests? ? detail
Usable by Peer Developers? ? detail
Code Secure? ? detail
Performance OK? ? detail
Deployment OK? ? detail
Copyright and Acknowledgements? ? detail
Happy to Maintain? ? detail

HeronLoadDev has adopted the pull request workflow:

The contributor requests that the project maintainer "pull" the source code change, hence the name "pull request". The maintainer has to merge the pull request if the contribution should become part of the source base. -- pull request in wikipedia

Even for projects that don't use github or git per se, we have a long tradition of using branches for development, with code review required for merging to master/default and closing the ticket. (Deployment is normally done in a separate "release" ticket for HERON, per policy established in #296.)

Shared Code Quality

Is this code I would be happy to maintain? That's the bottom line of our code review checklist. We're collectively responsible for the quality of our code, and any one of us may be called on to fix the next bug or add the next feature.

WritingQualityCode means no more untested, undocumented code.

Is the code DRY?

DRY is short for Don't Repeat Yourself.

When the DRY principle is applied successfully, a modification of any single element of a system does not require a change in other logically unrelated elements.

Copy and paste programming is the sometimes tempting, and sometimes even expedient, alternative.

Is unit test coverage adequate?

... especially for any code that didn't work the first time.

Note:

Q: (Why) is it important that a unit test not test dependencies?
A: It's a matter of definition. A test with dependencies is an integration test, not a unit test.

Clear Use Case

Is the use-case clear?

For details, see

Is the use-case reasonably well captured in automated tests?

Note: tests should tell a story; don't use names like foo and bar.

TODO: reconcile with "no more untested, undocumented code" in WritingQualityCode

Community Peer Perspective

  • Is it clear how to run the tests? which end of the code is up?
  • Could we expect our peers in the informatics community to use this code?
    • Note: references to tickets from code are OK, but only in a supplementary role; they should not be essential to understanding the purpose or design of the code.

External Design Constraints

  • Are external design constraints clear?
    • For example, is it clear which names were made up and which come from elsewhere?
      • One convention is to use the source column name when building concept codes. Actually, on review, this convention seems to have fallen into disuse long ago.
    • Are sources cited?
  • Is the copyright notice clear and correct?
    • If we're using anyone else's work, is it clearly licensed and acknowledged?
    • We did an audit in #265, but we don't have tool support since then.

Copyright notice in every file?

Dan's preference: unless we have tool support, don't bother

Discussion shows it's not critical, though it can be useful:

In short, you don't need to put the license in each file. ... There's no extra legal protection in doing so.

on the other hand:

... it's very easy to dis-aggregate a single source code file from its larger project, such as someone just checking out, emailing, downloading one file, without the rest that contains the full copyright. And then that file can get passed along ad-infinitum into time, to Nth parties who may have no idea of the files origins.

This is especially relevant since many of our source files are available as individual web pages.

Which markup style?

Somewhat conventional in the python community, but doesn't show up when looking at the module docstring:

__copyright__ = 'Copyright (c) 2012 University of Kansas Medical Center'

or in the docstring:

:copyright: Copyright 2014 University of Kansas Medical Center

how does sphinx render this?

Security and Least Authority

  • Is the code secure? Does it follow the principle of LeastAuthority? Is the flow of authority clear?

Note security keyword.

Deployment

  • Is installation, deployment reasonably well documented?
    • Can you reproduce the results, i.e. can you run integration tests?
    • Is logging sufficient to diagnose problems?
      • production logging level should be INFO or above; use log.debug() only for debugging

Performance

  • Has code been spot-checked for performance?
    • For example, have SQL views been run manually against large data sets (such as Clarity1) to quantify performance? Real-world examples where this could have helped us avoid delaying ETL (ticket:2595#comment:20, #1333)
    • Have the execution plans been checked for obviously poor plans?

Note keyword.

Issues

In addition to...

#2170
character encoding for HERON ETL source files

we could use some clarity on...

  • Python community norms are to use .rst format; e.g. source:heron_load/README.rst. But until we have a jenkins job that forces (or at least helps) us get it right, it's not clear that this is cost-effective.
  • 80 character line lengths are supported by PEP8 tools for python; adopt this convention elsewhere? e.g. README files?
  • Can we commit to ocap discipline (cf. AuthorityInjection) even without tool support?
  • anybody else (besides Dan) interested to try well-typed python, as in source:rgate/devdoc/well_typed.rst?

p.s. still pending? ticket:1386#comment:23 slicer code review notes

Formatting change logs for trac

This little recipe is handy:

hg log --style compact --limit 20 --template '  {rev}. [{node|short}] {desc|firstline}\n'|sort -n

See, for example, ticket:1663#comment:35 and ticket:2408#comment:3 .

or as we migrate to git:

git log -n 30 "--pretty=format:1. %H %s" | tac

Review i2b2 deployment scripts 31 Aug

The goal was to review Dan's work on source:i2b2-backend-deploy with Russ and Arvinder, but to understand the context, we first went through (and debugged) RavenPortalDevelopment, the process for other developers (Russ, in particular) to revise and re-deploy the RAVEN portal.

We went on to review source:i2b2-backend-deploy, using i2b2-backend-deploy dev docs generated from the source.

We discussed only briefly how the new scripted approach relates to the installation documentation in Using I2B2.

We didn't discuss #102 acks, support.

The generated docs are not (yet) automatically updated when code is committed. The manual process involved running sphinx via make and then updating the web pages with rsync.

Last modified 16 months ago Last modified on Jan 25, 2019 9:53:00 AM