chriscardillo / norris Goto Github PK

Hi Chris,

I saw this package pop up via Twitter. Going through the CRAN page I noticed a few dependencies which made me peek in the code a little.

I see that you Depdends: on a couple of packages. The preferred way is to Imports: rather than Depends:. This is because if you Depend on a number of packages, when a user loads norris, all packages are loaded for the user, even if they are only interested in norris::get_random_joke(). If you use Imports: the functions of the imported packages are only visible for norris, keeping the user's environment cleaner.

Also, I think you could do with lesser dependencies. For example, you use stringr to concatenate strings. Did you know you can use base R's paste0 for the same purpose? e.g in this line:

    random_joke_response <- httr::GET(stringr::str_c("http://api.icndb.com/jokes/random/", jokes_round, "?escape=javascript"))

you could also use

    random_joke_response <- httr::GET(paste0("http://api.icndb.com/jokes/random/", jokes_round, "?escape=javascript"))

In this case pasting is even unnecessary:

 random_joke_response <- httr::GET(stringr::str_c("http://api.icndb.com/jokes/random", "?escape=javascript"))

can be replaced with

 random_joke_response <- httr::GET("http://api.icndb.com/jokes/random?escape=javascript")

So you can loose the dependency on stringr without loss of functionality. One reason to want this is that stringr itself depends on the stringi package which is really useful, but also really large.

I see you also use dplyr as a dependency to create a tibble. Again, dplyr is a huge package, wich pulls in some big dependencies on its own. Since random_joke$value is already a data frame,
why not just return that? So here you can replace

return(random_joke$value %>% dplyr::as_tibble())

with

retrun( random_joke$value )

Finally, I would discourage the use of %>% within functions. The pipe operator is great for interactive use, but it does make debugging your functions really hard because it hides intermediate results. So for long-term maintenance of your package, you are better of by replacing things like

y <- x %>% fun1() %>% fun2()

with something like

y <- fun1(x)
y <- fun2(y)

Because now, if you have some error, you can very easily inspect intermediate results, for example with the interactive debug function.

As a demonstration of how the pipe operator can make debugging harder, you can try this example.
First I create a simple function that just throws an error.

f <- function(x){
   stop("foobar!")
}

Calling it gives an error. After the error I use the traceback() function, which tells me exactly where things went wrong (handy if you have functions calling functions calling functions ...)

> f(3)
Error in f(3) : foobar!
> traceback()
2: stop("foobar!") at #1
1: f(3)

The traceback clearly tells you where the error occurred. Now look what happens if I use the pipe operator:

> 3 %>% f()
Error in f(.) : foobar!
> traceback()
10: stop("foobar!") at #1
9: f(.)
8: function_list[[k]](value)
7: withVisible(function_list[[k]](value))
6: freduce(value, `_function_list`)
5: `_fseq`(`_lhs`)
4: eval(quote(`_fseq`(`_lhs`)), env, env)
3: eval(quote(`_fseq`(`_lhs`)), env, env)
2: withVisible(eval(quote(`_fseq`(`_lhs`)), env, env))
1: 3 %>% f()

The pipe operator adds 8 steps in the call stack. You can imagine what happens if you have a statement with multiple pipes in there.

Cheers, and happy coding!!
-Mark