CodeReviewRequest: SDGsR

DrMattG commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

SORTEE / peer-code-review