design
design copied to clipboard
tidyverse
Make it clear what you are branching over
is_foofy <- function(x) x %in% c("a", "b", "c")
# How do you make clear which cases need to be handled?
if (is_foofy(x)) {
if (x == "a") {
} else if (x == "b") {
} else if (x == "c") {
}
}
# How do you make the 3 cases clear?
n_unnamed <- sum(!named)
if (n_unnamed == 0) {
# do nothing
} else if (n_unnamed == 1) {
clauses <- c(clauses, build_sql("ELSE ", input[!named]))
} else {
stop("Can only have one unnamed (ELSE) input", call. = FALSE)
}
if (is.call(fun)) {
if (is_namespaced_dplyr_call(fun)) {
call[[1]] <- fun[[3]]
} else if (is_tidy_pronoun(fun)) {
stop("Use local() or remote() to force evaluation of functions", call. = FALSE)
} else {
return(eval_bare(call, env))
}
}
Unappealing because one clause continues and two exit
My approaches to these problems, for discussion:
is_foofy() seems redundant if we have is_foofy_a() etc.?
clauses <- c(clauses, else_clause(input[!named])
...
else_clause <- function(input) {
if (is_empty(input)) return()
if (!has_length(input, 1)) stop(...)
...
}
if (is.call(fun) && !is_namespaced_dplyr_call(fun)) {
if (is_tidy_pronoun(fun)) {
stop("Use local() or remote() to force evaluation of functions", call. = FALSE)
}
return(eval_bare(call, env))
}
call[[1]] <- fun[[3]]
The else or the shortcut via stop() or return() makes them mutually exclusive by definition. Do we need an else if there is also a shortcut?
I'm always taking a second look if a function has multiple if() clauses that are not connected with an else. Is this issue related to cyclomatic complexity of individual functions? Can we suggest that each function uses at most one control flow structure?
Is this related with the notion that "surprises" should be in branches, and the default control flow should describe the "regular" path?
# Good
if (surprise()) {
abort("oops")
}
carry_on()
# Bad
if (expected()) {
return(some_result())
}
abort("oops")
I don't think one style is good or bad, but mixing the two is bad. I regularly conclude functions with an exception. If you have a series of surprises, they should all exit non-locally, and If you have a series of expectations, they should all exit locally? With the obvious exception of the type checking at the beginning of the function with non-local exits, which should be taken as its own paragraph inside the function.
I'm working on a good example where this feels rather desirable (and unavoidable):
if (expected()) {
return(some_result())
}
abort("oops")
It's a function to process HTTP responses. The case of status 200, JSON Content-Type is easy to handle and you confirm and exit ASAP for that. Then we go on to throw various sorts of errors for all the other weird stuff that happens. In any case, it's a good type of task to bear in mind here.
