epiforecasts
Functions Problems
In general, the function arguments are poorly named.
The are often too long.
They have similar prefixes (e.g., num.initial.cases and num.initial.clusters), which can present problems with R's ability to partially match named arguments.
They don't lean on the nomenclature established by the language ecosystem (e.g., incfn could be rIncubation to leverage people's familiarity with runif, rexp, etc.).
There are several functions with lots of arguments. Almost always an indication of doing to much in a function, and thus needing to change steps and how data flows between them.
Quick update on this issue while working on the package.
the function arguments are poorly named. ... The are often too long.
The naming of function arguments should be improved in #112. On the length of function arguments, I tend to prefer longer names that are clearer, rather than abbreviations that may be misunderstood. There is of course a balance to this, I usually go with the {lintr} limit of 30 characters.
They have similar prefixes (e.g., num.initial.cases and num.initial.clusters), which can present problems with R's ability to partially match named arguments.
num.initial.clusters is no longer an argument in the package, but there are still functions with similar prefixes. Some function argument names were updated in #112, while addressing #65 would likely superseded this issue.
They don't lean on the nomenclature established by the language ecosystem (e.g., incfn could be rIncubation to leverage people's familiarity with runif, rexp, etc.).
incfn has been renamed to incubation_period in #84.
There are several functions with lots of arguments. Almost always an indication of doing to much in a function, and thus needing to change steps and how data flows between them.
I think this can be addressed via #65.
One function I think does need renaming to be more informative is inf_fn().
inf_fn() is renamed to incubation_to_generation_time() and it's arguments have been renamed to incubation_period_samples and alpha in PR #123. The parameterisation of presymptomatic transmission has also been improved in #123 updating k to prop_presymptomatic in outbreak_model() and scenario_sim().
@pearsonca are you happy for this issue to be closed?
Would want to double check the argument prefix collision part, but I think the rest has been addressed. Could definitely spin off whatever might remain into a more targeted issue.
There are several functions with lots of arguments. Almost always an indication of doing to much in a function, and thus needing to change steps and how data flows between them.
The outbreak simulation functions have been refactored in PR #127 to reduce the number of arguments and use helper functions: delay_opts(), event_prob_opts(), intervention_opts(), offspring_opts(), and sim_opts().
Therefore, everything raised in the initial issue comment has now been addressed from my perspective. If there are additional issues related to the functions or arguments these can be opened in new issues.