-
Notifications
You must be signed in to change notification settings - Fork 4
Self code review
Following is a checklist of common issues that can be checked by a developer prior to requesting code review.
Any time you find yourself copy/pasting lines of code, ask yourself immediately if the repeated code can be factored out. Sometimes it may be less obvious but DRY still applies, for example:
if dat is not None:
dat.write(Path(data_root) / "notes.csv", format="ascii.csv", overwrite=True)
else:
dat = Table.read(Path(data_root) / "notes.csv", format="ascii.csv")
Here Path(data_root) / "notes.csv" is repeated. This is bad because it requires checking that the file path in the two cases is actually the same instead of single-sourcing it.
# Using a "_path" suffix can be handy to remember it is a path,
# though VS code will also tell you.
notes_path = Path(data_root) / "notes.csv"
if dat is not None:
10000
dat.write(notes_path, format="ascii.csv", overwrite=True)
else
dat = Table.read(notes_path, format="ascii.csv")
The most common example are CxoTimeLike arguments, which should be immediately converted to CxoTime like start = CxoTime(start).
Another common case is a directory (str, but maybe a Path). Since Path is so handy, just cast the directory as a Path. From that point you can safely use it for a file path like out_dir / "notes.csv".
The following repeatedly casts npnt_start as CxoTime, and more problematically does CxoTime(scs107_intervals["datestart"]) for every loop iteration even though the result does not change each loop.
for row in npnt:
npnt_start = row["datestart"]
npnt_stop = row["datestop"]
if np.any(
(CxoTime(npnt_start) >= CxoTime(scs107_intervals["datestart"]))
& (CxoTime(npnt_start) <= CxoTime(scs107_intervals["datestop"]))
):
continue
# If down to less than a minute, skip
if CxoTime(npnt_stop) - CxoTime(npnt_start) < 60 * u.s:
continue
Instead, cast in advance, and if you have an API guarantee on the format of a date then take advantage of that. In this case we know that scs107_intervals["datestart"] is a date string.
for row in npnt:
npnt_start = CxoTime(row["datestart"])
npnt_stop = CxoTime(row["datestop"])
# If the start of the interval is within an SCS107 interval, skip this row
if np.any(
(npnt_start.date >= scs107_intervals["datestart"])
& (npnt_start.date <= scs107_intervals["datestop"])
):
continue
# If down to less than a minute, skip
if npnt_stop - npnt_start < 60 * u.s:
continue
If dat is an astropy Table and ok is some bool mask for selecting elements, then:
dat[colname][ok] # YES: fast
dat[ok][colname] # NO: makes a full new table first.
This is a variation of DRY to avoid repeating the name of a variable being defined:
if bool_value:
value = something
else:
value = something_else
As long as the bool_value, something, and something_else are each shorter than a single line. Ruff will format long variations nicely, like:
value = (
this_is_a_pretty_long_expression
if (some_condition and some_other_condition)
else this_is_a_different_long_expression
)
Using parentheses around the conditional expression is encouraged if it improves clarity.