Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce code duplication by using more general functions in the back-end #39

Closed
richelbilderbeek opened this issue Jul 21, 2018 · 3 comments

Comments

@richelbilderbeek
Copy link
Member

Feedback bjoelle:

Structure

  • The structure is my main concern with this package, as it contains a lot of code duplication and many functions need to run through all possible types of a certain object to figure out what to do with it. I think this makes the package hard to maintain and to extend to additional models.

The main issue [...] (see #38)

Separating the different parameters (alpha, beta, gamma, etc) also leads to a lot of code duplication. It is useful for creating parameters as their default priors are different, but I would remove it for the other functions: the many functions is_%sth%_param(x) can be replaced by one function is_named_param(x, name) and the parameter_to_xml functions also have a lot of redundancy.

@richelbilderbeek
Copy link
Member Author

richelbilderbeek commented Oct 25, 2018

For the is_*_param case: bjoelle has a point, see for example:

is_alpha_param <- function(
  x
) {
  if (!is_param(x)) return(FALSE)
  x$name == "alpha"
}

I do easily see that I can reduce those two lines to one, but I do not think it's worth it for three reasons:

  1. the functions this applies to are still trivially simple
  2. not all functions are as simple, for example:
is_kappa_1_param <- function(
  x
) {
  if (!is_param(x)) return(FALSE)
  if (x$name != "kappa_1") return(FALSE)

  if (!"lower" %in% names(x)) return(FALSE)
  if (!"value" %in% names(x)) return(FALSE)
  if (!"estimate" %in% names(x)) return(FALSE)
  TRUE
}
  1. It reflects the current state of the package: not all parameters have a lower, upper, or estimate argument yet

I do predict in the future these is_*_param functions will get the same shape. At the current state, I think it's not worth it to refactor the is_*_param functions.

@richelbilderbeek
Copy link
Member Author

richelbilderbeek commented Oct 25, 2018

Likewise for parameter_to_xml_*: it reflects the state of the package.

Note how this ties in with #45.

@richelbilderbeek
Copy link
Member Author

I feel that bjoelle is right, yet it should and can be done as suggested if and only if the package is in a finished state. Because the package is not yet complete, this Issue must be moved to the future and potentially re-opened then.

Process rOpenSci feedback automation moved this from To do to Done Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

1 participant