CodeReviewRequest: SDGsR

[DrMattG]

Author: DrMattG
Repo: https://github.com/DrMattG/SDGsR
Aim: To maximise the usefulness of this R package for others. I developed this for a very specific use case - but I think it will be useful for other people who want to interact with the SDGs API. I really do not know what I am doing when it comes to APIs and getting all the data in a useful format has been (and continues to be) challenging. This maybe a bit outside of the normal code review - there is no rush for this. From this review I would like:

• some suggestions on improving code in the functions
• some suggestions on increasing the generalisability of the code to other use cases
• some help in understanding the error/warning/note messages from devtools::check()
• some suggestions for additional functions
• some ideas about what to do with the package - leave it on github, submit to CRAN, give up....?

File Info: The repo contains an R package with the functions stored in the "R" folder. There are folders associated with the packagedown site (https://drmattg.github.io/SDGsR/) that can be ignored.

[Aariq]

This seems like it might be appropriate for rOpenSci review, if you're interested at some point. They've got an active Slack that can help with your devtools::check() issues, and the review is a really great learning experience.

[DrMattG]

This seems like it might be appropriate for rOpenSci review, if you're interested at some point. They've got an active Slack that can help with your devtools::check() issues, and the review is a really great learning experience.

Thanks @Aariq! Do you think it is something that rOpenSci would consider?

[Aariq]

You can submit a pre-submission inquiry by opening an issue here: https://github.com/ropensci/software-review/issues
They'll let you know if it's in-scope. It sounds like it would be to me though.

[Aariq]

I missed the SORTEE session about code review, so forgive me if I'm not following some format that was decided upon, but I have some ideas:

DESCRIPTION

• For the Licence, you want to actually run use_gpl3_licence() in the console. It will edit your DESCRIPTION and add a LICENCE.md file
• Looks like you import tidyverse, but I'm guessing not every package in tidyverse is necessary for your package. I'd avoid importing all of tidyverse in a package.

devtools::check() results

• I think you want to keep SDGs15.csv out of the data directory. You could also add it to .Rbuildignore

Functions

• get_SDGs_goals() should probably just return the dataframe and leave the choice of whether to save the data to the user. If you do want it to write data, it should probably take some kind of path argument rather than writing the file to the working directory. It would also be nice if it printed a message like "'SDGS_all.rds' saved to ".
• get_SDGs_goals() could probably benefit from vectorization with lapply() or purrr::map_df()—try writing the function so that it downloads just one of the URLs, with an argument that fills in the number 1 through 17, then you can apply that function to 1:17 and combine the results into a list.
• for lookup_country(), I'd put the code="m49" argument second. Arguments with defaults usually go at the end so that it's easier to just do lookup_country("Yemen"), for example, if you are OK with the default for code. Currently lookup_country("Yemen") would give an error.
• get_indicator_example_data() currently runs library(tidyverse). This is probably just a mistake, since you've used the package::function() syntax everywhere already, but you shouldn't have functions that call library() in an R package.
• it seems like the indicator argument for get_indicator_data() and get_indicator() shouldn't have a default, or if it does, it should be more like c("15.6.1", "16.3.4", "18") (forgive me, I don't know what values are possible). If you have a vector as a default, you can use match.arg() in your function to check that only one of the allowed values is chosen, and if the user supplies nothing, then it picks the first value as the default. It also shows up a lot nice in tooltips and documentation, IMO.
• runExample() if there is only one example currently, then it makes sense to have example = "indicator.R" as a default. It might be good to use require() in this function to check that the user has shiny installed. (also, this function didn't work for me—not sure why)

Documentation

• The "title" of functions should be more than just the function itself. E.g. instead of get_indicator_example_data you might call it "Get example indicator data from SDGs"

You're definitely on your way to having something you could submit to CRAN, but you'll have to make sure you get no warnings on devtools::check() before they'll consider it.