Do not use else after stop()

[Bisaloo]

In the words of Jenny Bryan: "there is no else, there is only if."

https://speakerdeck.com/jennybc/code-smells-and-feels?slide=45

Having else after return() of stop() increases the number of branches in the code, which makes it harder to read. It also translates into a higher cyclomatic complexity.

[codecov]

Codecov Report

Merging #229 (9df6e8b) into develop (d21ab56) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 9df6e8b differs from pull request most recent head ee5cad3. Consider uploading reports for the commit ee5cad3 to get more accurate results

@@           Coverage Diff            @@
##           develop     #229   +/-   ##
========================================
  Coverage    97.31%   97.31%           
========================================
  Files           14       14           
  Lines         1527     1527           
========================================
  Hits          1486     1486           
  Misses          41       41           

Impacted Files	Coverage Δ
R/check.R	100.00% <100.00%> (ø)
R/preprocess.R	96.44% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[seabbs]

Haha, nice! Standard comment about a news item and dev version increment otherwise looks good to me.

[pearsonca]

disagree with this notion as a rule of thumb (and apparently, the cyclomatic complexity metric, if it triggers on this sort of thing).

in this particular case, i think the stopifnot named argument idiom would be better anyway:

stopifnot(
  "error1text" = checkcondition1,
  "error2text" = checkcondition2,
  ...
)

[Bisaloo]

I double checked and indeed, it doesn't affect cyclomatic complexity but I still believe this is a good change that makes the code easier to read.

cyclocomp::cyclocomp(function() {
  if (a) {
    stop("error") 
  } else {
    message("good")
  }
})
#> [1] 2

cyclocomp::cyclocomp(function() {
  if (a) {
    stop("error") 
  }
  message("good")
})
#> [1] 2

^{Created on 2023-04-12 with reprex v2.0.2.9000}

[pearsonca]

For error-checking / messages, i think this generally reads more clearly (comments for emphasis - they're unnecessary imo, especially if this idiom is repeated through a library)

stopifnot( # ... we're doing some error checking
  "checking this feature" = ..., # against some logical expression
  ... # repeat for all the desired checks
)

[pearsonca]

Aside: I am on-board with the don't-if-else-your-errors mentality, just not necessarily the return() bit. There are two useful idioms for returns in branching code:

• pre declare the object to manipulate, make the manipulations in the different branches, return after the branches.
• have each branch return at its end

I have a slight preference for the first, but its not always practical, nor is it always more readable. As such, disagree with making it a general principle / practice - that's for strong preferences, particularly where the lazy alternatives pose distinct downsides.

[pearsonca]

looking at the linked JB example, agree the LHS is code smell, but again: the stopifnot(...) idiom would be a much clearer solution than the RHS. That idiom telegraphs: "Here are all the things we can check up front and immediately exit, for reasons".

However, sometimes there's actually some branching logic (e.g. when embracing some polymorphism in function arguments, though yes that too can go overboard), and you can't properly check exit conditions until you've made some progress on evaluating arguments.

[Bisaloo]

I agree that stopifnot() is easier to understand for simple checking. I can do the change for the case at hand.

[seabbs]

Everyone agrees it appears the original is not ideal.

The choice is between using individual if and stop checks or doing a single stopifnot. I don't have a strong preference here. @pearsonca could you draft a suggested version using stopifnot so we can compare?

Alternatively, we can merge this PR and then look at moving to a stopifnot flow in a future PR (as deciding on this doesn't prevent us from improving the current implementation).

[pearsonca]

Have y'all stood up style guidelines or some such? I think the practical solution is roughly:

• do this PR now (agree, generally better + doesn't make move to stopifnot harder)
• have those style guidelines in place
• have a issue(s) => PR(s) exclusively focused on implementing guidance item(s), including this one

[seabbs]

Have y'all stood up style guidelines or some such

There are (in the contributing guide) but they are quite vague.

[Bisaloo]

Maintaining a thorough style guide is so much work that I usually try to stick with existing ones, even if I disagree with some of their choices (as long as we still agree on most points obviously).

[TimTaylor]

Unsolicited, 😬, but hopefully useful comments:

• stopifnot() definitely looks nice.
• stopifnot() is restrictive in that you cannot suppress the calling function as with stop() nor can you return condition objects with custom classes. Both of these can be useful with internal check functions.
• There is a slight overhead to successful stopifnot() calls that is avoided with stop() (only relevant if the function is called a lot and normally can be avoided by better structuring of code in this case anyway).

How much the last two points matter depends on the use case but worth bearing in mind.

[pearsonca]

Maintaining a thorough style guide is so much work that I usually try to stick with existing ones, even if I disagree with some of their choices (as long as we still agree on most points obviously).

Doesn't have to be complete (e.g. "the specified lintr config passes" covers a lot), and this is more of a "style" guide anyway. I think it's roughly at the moment:

• generally, name exported functions with enw_..., and don't do that for internal functions
• do check arguments on externally facing functions, don't on internal ones
• require data.frame only for inputs, indicate data.table for outputs
• (as of this discussion) check as many arguments at the outset of enw_ with stopifnot(...) block

Others?

I do think the contributing guidelines currently are too focused on "process" - that's useful, but it's distinctly worthwhile to establish what are the "vocabulary" or "idioms" for enw. That's has some elements of "style" guidelines, but is generally a bit more meta (i.e. human-enforceable only) than concrete (i.e. static analysis works fine).

[seabbs]

Doesn't have to be complete (e.g. "the specified lintr config passes" covers a lot), and this is more of a "style" guide anyway. I think it's roughly at the moment

These are some good points @pearsonca. Shall we move this discussion to an issue ahead of updating the contributing guide?

@TimTaylor !!! (all good points)

I agree with @Bisaloo when it comes to trying to stick with existing guides due to the amount of work (which is what our current contributing guide says). I think we have diverged a bit here so perhaps we need to update on some specific points and then point out to existing guides for everything else that doesn't contradict?

[pearsonca]

In terms of actual code, for check_date, I'd prefer something akin to:

with(obs, stopifnot(
  "Both `reference_date` and `report_date` must be columns." =
     !(is.null(reference_date) && is.null(report_date)),
  "`reference_date` must be a column." = !is.null(reference_date),
  "`report_date` must be a column." = !is.null(report_date),
 local = ... # not quite sure here; seems like this is an internal function used for checks - parent.frame?
))

probably before the internal copy steps as well?

[TimTaylor]

For a stop() approach I've used something similar to the following for internal check functions:

.stopf <- function(fmt, ..., .use_call = TRUE, .call = sys.call(-1L), .class = NULL) {
    .call <- if (isTRUE(.use_call)) .call[1L] else NULL
    msg <- sprintf(fmt, ...)
    err <- errorCondition(msg, class = .class, call = .call)
    stop(err)
}

.assert_not_missing <- function(x, arg, call) {
    if (missing(x))
        .stopf("argument `%s` is missing, with no default.", arg, .call = call)
}


.positive_int <- function(x, arg = deparse(substitute(x)), call = sys.call(-1L)) {
    .assert_not_missing(x, arg = arg, call = call)
    if (!is.integer(x) || x <= 0L)
        .stopf("`%s` must be a positive integer.", arg, .call = call)
}


external <- function(bob) {
    .positive_int(bob)
}

# will work
external(1L)

# will error
external(1)
#> Error in external(): `bob` must be a positive integer.
external(0L)
#> Error in external(): `bob` must be a positive integer.
external()
#> Error in external(): argument `bob` is missing, with no default.

^{Created on 2023-04-12 with reprex v2.0.2}

The first two functions are the boiler plate stuff with the .positive_int() being akin to your check_dates(). It is very easy to mess up which frame to evaluate things in so you do do need to take care 😅 . {rlang} does have a lot for this sort of thing should you wish to go that route.

^{Created on 2023-04-12 with reprex v2.0.2}

[seabbs]

Right so I think where we are with this PR is to take forward @Bisaloo suggests as is then open an issue to take forward the general argument checking approach. I like the move to stopifnot but as @TimTaylor suggests some clear issues. Also like @TimTaylor skeletonised approach which we could adopt and if anyone has thoughts about parts of rlang we could use we could consider as a dependency?

Would anyone be happy to write up an issue for this and port over some of the discussion?

Ah we also have a suggested extension to the style guide. @pearsonca could you take point on porting that part of the discussion to its own issue?

We can then discuss more on both and add to a version roadmap + unstick this PR!

[seabbs]

LGTM to me. I have added a news item and bumped the dev version. Porting the outstanding conversation here to two new issues. Thanks again for this PR @Bisaloo and the great discussion @pearsonca, @TimTaylor, and @Bisaloo!

[seabbs]

#233 captures the discussion here on approaches for improving input checking.

[seabbs]

#234 captures the contributing guide discussion.