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

enw_formula_as_data_list()s implementation could be improved to better align with DRY principles #245

Closed
pearsonca opened this issue Apr 21, 2023 · 3 comments · Fixed by #254
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@pearsonca
Copy link
Collaborator

pearsonca commented Apr 21, 2023

enw_formula_as_data_list is a bit convoluted and breaks the DRY (don't repeat yourself) principle:

enw_formula_as_data_list <- function(formula, prefix,
drop_intercept = FALSE) {
paste_lab <- function(string, lab = prefix) {
paste0(lab, "_", string)
}
if (!missing(formula)) {
if (!inherits(formula, "enw_formula")) {
stop(
"formula must be an object of class enw_formula as produced using
enw_formula"
)
}
fintercept <- as.numeric(any(grepl(
"(Intercept)", colnames(formula$fixed$design), fixed = TRUE
)))
data <- list()
data[[paste_lab("fdesign")]] <- formula$fixed$design
data[[paste_lab("fintercept")]] <- fintercept
data[[paste_lab("fnrow")]] <- nrow(formula$fixed$design)
data[[paste_lab("findex")]] <- formula$fixed$index
data[[paste_lab("fnindex")]] <- length(formula$fixed$index)
data[[paste_lab("fncol")]] <-
ncol(formula$fixed$design) -
min(as.numeric(drop_intercept), as.numeric(fintercept))
data[[paste_lab("rdesign")]] <- formula$random$design
data[[paste_lab("rncol")]] <- ncol(formula$random$design) - 1
} else {
data <- list()
data[[paste_lab("fdesign")]] <- numeric(0)
data[[paste_lab("fintercept")]] <- 0
data[[paste_lab("fnrow")]] <- 0
data[[paste_lab("findex")]] <- numeric(0)
data[[paste_lab("fnindex")]] <- 0
data[[paste_lab("fncol")]] <- 0
data[[paste_lab("rdesign")]] <- numeric(0)
data[[paste_lab("rncol")]] <- 0
}
return(data)
}

Seems would be clearer as roughly or similar:

# ...initial checking stuff ...
data <- list()
data[c("fintercept", "the other pertinent variables")] <- 0
data[c("fdesign", "these other pertinent variables") <- list(numeric())
if (!missing(formula)) {
  data$fdesign <- formula$fixed$design
   # ... etc
}
names(data) <- sprintf("%s_%s", prefix, names(data))
return(data)

Updating this would make the code easier to maintain and expand in the future.

@seabbs seabbs added the enhancement New feature or request label Apr 21, 2023
@seabbs
Copy link
Collaborator

seabbs commented Apr 21, 2023

Thanks for this @pearsonca

I agree the current implementation can be shortened though and this looks like a workable way of doing it.

Would you like to take this on? If we act fast then we can get in it 0.2.1. Also sharing more widely as this is a good starter issue!

@seabbs seabbs added the good first issue Good for newcomers label Apr 21, 2023
@pearsonca
Copy link
Collaborator Author

Perhaps leave it for another increment, and take advantage of the "good first issue"ness of it.

@seabbs seabbs changed the title DRY enw_formula_as_data_list enw_formula_as_data_list()s implementation could be improved to better align with DRY principles Apr 22, 2023
Lnrivas added a commit to Lnrivas/epinowcast that referenced this issue Apr 24, 2023
@seabbs seabbs linked a pull request Apr 24, 2023 that will close this issue
@seabbs
Copy link
Collaborator

seabbs commented Apr 24, 2023

Note that @Lnrivas has a draft PR seeking to address this issue here #249. Potentially help still needed supporting this if interested.

Lnrivas added a commit to Lnrivas/epinowcast that referenced this issue Apr 24, 2023
Lnrivas added a commit to Lnrivas/epinowcast that referenced this issue Apr 24, 2023
seabbs pushed a commit to Lnrivas/epinowcast that referenced this issue Apr 25, 2023
seabbs pushed a commit to Lnrivas/epinowcast that referenced this issue Apr 25, 2023
seabbs added a commit that referenced this issue Apr 25, 2023
…nciples (#245) (#254)

* Issue #245

* New contributor Lnrivas

* Issue 245: Cleaning enw_formula_as_data_list() (#245)

* fixed spacing

* match original spacing style

* Update R/model-tools.R to fix linting issues

Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>

* Add @Bisaloo as a reviewer

* Dropped news note about contributor change as we haven't been tracking

* Debug @seabbs rebasing

* reorder data list to match original code

* dropped unneeded lint gates

* turn into a normal list

---------

Co-authored-by: Hugo Gruson <Bisaloo@users.noreply.github.com>
Co-authored-by: Sam <s.e.abbott12@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
No open projects
Status: Done
2 participants