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

Replace loop with more functional code #168

Closed
RayStick opened this issue Dec 17, 2024 · 4 comments
Closed

Replace loop with more functional code #168

RayStick opened this issue Dec 17, 2024 · 4 comments
Assignees
Labels
enhancement Feature improvement or addition

Comments

@RayStick
Copy link
Member

RayStick commented Dec 17, 2024

Suggestion that you do not have to tackle now since reviewers might have an opinion too.

Could the for loop be replaced with more functional code where the name of functions might help understand what's being calculated.

In R/data_manipulation.R:

is_empty_description <- function(index, table) {
  nchar(table$description[index] ||
  table$description[index] == "Description to follow") ||
  table$description[index] == "NA" # why doesn't  `is.na()` work by the way?
}
# untested, and might be wrong
table$description <- purrr::map_lgl(seq_len(nrow(table_df)), is_empty_description, table = table)
# this returns a logical not character

Originally posted by @maelle in #167 (comment)

@RayStick RayStick changed the title Suggestion that you do not have to tackle now since reviewers might have an opinion too. Replace loop with more functional code Dec 17, 2024
@RayStick RayStick added this to the rOpenSci Review Process milestone Dec 17, 2024
@RayStick RayStick added the enhancement Feature improvement or addition label Dec 17, 2024
@RayStick RayStick mentioned this issue Dec 17, 2024
16 tasks
@maelle
Copy link
Collaborator

maelle commented Dec 18, 2024

Just thought that

table$description[index] == "Description to follow") ||
  table$description[index] == "NA"

could be

table$description[index] %in% c("Description to follow", "NA")

which is vectorized

descriptions <- c(
  "real one", 
  "1", 
  "a",
  "-",
  "NA", 
  "Description to follow"
)
descriptions %in% c("NA", "Description to follow")
#> [1] FALSE FALSE FALSE FALSE  TRUE  TRUE

Created on 2024-12-18 with reprex v2.1.1

To also have the condition on the number of characters:

descriptions <- c(
  "real one", 
  "1", 
  "a",
  "-",
  "NA", 
  "Description to follow"
)

(nchar(descriptions) == 1) | descriptions %in% c("NA", "Description to follow")
#> [1] FALSE  TRUE  TRUE  TRUE  TRUE  TRUE

Created on 2024-12-18 with reprex v2.1.1

@maelle
Copy link
Collaborator

maelle commented Dec 18, 2024

So I think something like this should work:

table$empty <- (nchar(table_df$description) == 1) | table_df$description %in% c("NA", "Description to follow")

@RayStick
Copy link
Member Author

See this line here:

empty_count = sum((nchar(as.character(Column.description)) <= 1) | Column.description %in% c("NA", "Description to follow", NA)),

The code structure was changed a bit but I still used your suggestion as inspiration to simplify 😸

@maelle
Copy link
Collaborator

maelle commented Dec 20, 2024

Yay, dplyr for the win! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature improvement or addition
Projects
None yet
Development

No branches or pull requests

2 participants