JOSS Review: Suggested changes from {goodpractice}

njtierney commented 6 months ago

These might seem like minor points, but this R package could really benefit from making changes from the {goodpractice} R package:

goodpractice::gp()
#> Preparing: covr
#> Preparing: cyclocomp
#> ── R CMD build ─────────────────────────────────────────────────────────────────
#> * checking for file ‘/private/var/folders/9c/k3wqmhhx4qsb3fd66n4prhlw0000gq/T/Rtmp3U9jTS/remotes398e1dc20f3/gsMAMS/DESCRIPTION’ ... OK
#> * preparing ‘gsMAMS’:
#> * checking DESCRIPTION meta-information ... OK
#> * checking for LF line-endings in source and make files and shell scripts
#> * checking for empty or unneeded directories
#> * building ‘gsMAMS_0.7.1.tar.gz’
#> Adding 'gsMAMS_0.7.1.tgz' to the cache
#> Preparing: description
#> Preparing: lintr
#> Preparing: namespace
#> Preparing: rcmdcheck
#> ── GP gsMAMS ───────────────────────────────────────────────────────────────────
#> 
#> It is good practice to
#> 
#>   ✖ write unit tests for all functions, and all package code in
#>     general. 0% of code lines are covered by test cases.
#> 
#>     R/design_cont.R:18:NA
#>     R/design_cont.R:19:NA
#>     R/design_cont.R:20:NA
#>     R/design_cont.R:21:NA
#>     R/design_cont.R:22:NA
#>     ... and 1453 more lines
#> 
#>   ✖ write short and simple functions. These functions have high
#>     cyclomatic complexity (>50): op_power_ord (91), op_power_cont (86),
#>     op_fwer_ord (82), op_fwer_cont (77), op_power_surv (73),
#>     op_fwer_surv (64), Size_surv (54). You can make them easier to
#>     reason about by encapsulating distinct steps of your function into
#>     subfunctions.
#>   ✖ add a "URL" field to DESCRIPTION. It helps users find information
#>     about your package online. If your package does not have a
#>     homepage, add an URL to GitHub, or the CRAN package package page.
#>   ✖ add a "BugReports" field to DESCRIPTION, and point it to a bug
#>     tracker. Many online code hosting services provide bug trackers for
#>     free, https://github.com, https://gitlab.com, etc.
#>   ✖ use '<-' for assignment instead of '='. '<-' is the standard, and R
#>     users and developers are used it and it is easier to read your code
#>     for them if you use '<-'.
#> 
#>     R/design_cont.R:31:4
#>     R/design_ord.R:17:11
#>     R/design_ord.R:18:4
#>     R/design_ord.R:30:4
#>     R/design_surv.R:21:12
#>     ... and 619 more lines
#> 
#>   ✖ avoid long code lines, it is bad for readability. Also, many people
#>     prefer editor windows that are about 80 characters wide. Try make
#>     your lines shorter than 80 characters
#> 
#>     R/design_cont.R:3:81
#>     R/design_cont.R:10:81
#>     R/design_cont.R:12:81
#>     R/design_cont.R:20:81
#>     R/design_cont.R:31:81
#>     ... and 176 more lines
#> 
#>   ✖ omit trailing semicolons from code lines. They are not needed and
#>     most R coding standards forbid them
#> 
#>     R/op_fwer_surv.R:156:56
#>     R/op_power_surv.R:154:56
#> 
#>   ✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...), 1:NROW(...) and
#>     1:NCOL(...) expressions. They are error prone and result 1:0 if the
#>     expression on the right hand side is zero. Use seq_len() or
#>     seq_along() instead.
#> 
#>     R/design_cont.R:21:32
#>     R/design_cont.R:27:33
#>     R/design_ord.R:21:32
#>     R/design_ord.R:26:33
#>     R/design_surv.R:25:32
#>     ... and 17 more lines
#> 
#>   ✖ not import packages as a whole, as this can cause name clashes
#>     between the imported packages. Instead, import only the specific
#>     functions you need.
#>   ✖ fix this R CMD check NOTE: Found the following hidden files and
#>     directories: .github These were most likely included in error. See
#>     section ‘Package structure’ in the ‘Writing R Extensions’ manual.
#>   ✖ avoid 'T' and 'F', as they are just variables which are set to the
#>     logicals 'TRUE' and 'FALSE' by default, but are not reserved words
#>     and hence can be overwritten by the user.  Hence, one should always
#>     use 'TRUE' and 'FALSE' for the logicals.
#> 
#>     R/op_fwer_ord.R:NA:NA
#>     R/op_fwer_ord.R:NA:NA
#>     R/op_fwer_ord.R:NA:NA
#>     R/op_fwer_ord.R:NA:NA
#>     R/op_power_ord.R:NA:NA
#>     ... and 3 more lines
#> 
#> ────────────────────────────────────────────────────────────────────────────────

^{Created on 2024-03-25 with reprex v2.1.0}

Session info

``` r sessioninfo::session_info() #> ─ Session info ─────────────────────────────────────────────────────────────── #> setting value #> version R version 4.3.3 (2024-02-29) #> os macOS Sonoma 14.3.1 #> system aarch64, darwin20 #> ui X11 #> language (EN) #> collate en_US.UTF-8 #> ctype en_US.UTF-8 #> tz Australia/Brisbane #> date 2024-03-25 #> pandoc 3.1.1 @ /Applications/RStudio.app/Contents/Resources/app/quarto/bin/tools/ (via rmarkdown) #> #> ─ Packages ─────────────────────────────────────────────────────────────────── #> package * version date (UTC) lib source #> backports 1.4.1 2021-12-13 [1] CRAN (R 4.3.0) #> bit 4.0.5 2022-11-15 [1] CRAN (R 4.3.0) #> bit64 4.0.5 2020-08-30 [1] CRAN (R 4.3.0) #> blob 1.2.4 2023-03-17 [1] CRAN (R 4.3.0) #> cachem 1.0.8 2023-05-01 [1] CRAN (R 4.3.0) #> callr 3.7.5 2024-02-19 [1] CRAN (R 4.3.1) #> cli 3.6.2 2023-12-11 [1] CRAN (R 4.3.1) #> clisymbols 1.2.0 2017-05-21 [2] CRAN (R 4.3.0) #> covr 3.6.4 2023-11-09 [2] CRAN (R 4.3.1) #> crancache 0.0.0.9001 2024-01-12 [2] Github (r-lib/crancache@fc51e31) #> cranlike 1.0.2 2018-11-26 [2] CRAN (R 4.3.0) #> crayon 1.5.2 2022-09-29 [1] CRAN (R 4.3.0) #> curl 5.2.0 2023-12-08 [1] CRAN (R 4.3.1) #> cyclocomp 1.1.1 2023-08-30 [2] CRAN (R 4.3.0) #> data.table 1.15.0 2024-01-30 [1] CRAN (R 4.3.1) #> DBI 1.2.2 2024-02-16 [1] CRAN (R 4.3.1) #> debugme 1.1.0 2017-10-22 [2] CRAN (R 4.3.0) #> desc 1.4.3 2023-12-10 [1] CRAN (R 4.3.1) #> digest 0.6.34 2024-01-11 [1] CRAN (R 4.3.1) #> evaluate 0.23 2023-11-01 [1] CRAN (R 4.3.1) #> fansi 1.0.6 2023-12-08 [1] CRAN (R 4.3.1) #> fastmap 1.1.1 2023-02-24 [1] CRAN (R 4.3.0) #> fs 1.6.3 2023-07-20 [1] CRAN (R 4.3.0) #> glue 1.7.0 2024-01-09 [1] CRAN (R 4.3.1) #> goodpractice 1.0.4 2022-08-30 [2] CRAN (R 4.3.0) #> htmltools 0.5.7 2023-11-03 [1] CRAN (R 4.3.1) #> httr 1.4.7 2023-08-15 [1] CRAN (R 4.3.0) #> jsonlite 1.8.8 2023-12-04 [1] CRAN (R 4.3.1) #> knitr 1.45 2023-10-30 [1] CRAN (R 4.3.1) #> lazyeval 0.2.2 2019-03-15 [1] CRAN (R 4.3.0) #> lifecycle 1.0.4 2023-11-07 [1] CRAN (R 4.3.1) #> lintr 3.1.1 2023-11-07 [2] CRAN (R 4.3.1) #> magrittr 2.0.3 2022-03-30 [1] CRAN (R 4.3.0) #> memoise 2.0.1.9000 2024-02-12 [1] Github (hadley/memoise@40db995) #> parsedate 1.3.1 2022-10-27 [1] CRAN (R 4.3.0) #> pillar 1.9.0 2023-03-22 [1] CRAN (R 4.3.0) #> pkgbuild 1.4.3 2023-12-10 [1] CRAN (R 4.3.1) #> pkgconfig 2.0.3 2019-09-22 [1] CRAN (R 4.3.0) #> praise 1.0.0 2015-08-11 [1] CRAN (R 4.3.0) #> prettyunits 1.2.0 2023-09-24 [1] CRAN (R 4.3.1) #> processx 3.8.3 2023-12-10 [1] CRAN (R 4.3.1) #> ps 1.7.6 2024-01-18 [1] CRAN (R 4.3.1) #> purrr 1.0.2 2023-08-10 [1] CRAN (R 4.3.0) #> R.cache 0.16.0 2022-07-21 [2] CRAN (R 4.3.0) #> R.methodsS3 1.8.2 2022-06-13 [2] CRAN (R 4.3.0) #> R.oo 1.26.0 2024-01-24 [2] CRAN (R 4.3.1) #> R.utils 2.12.3 2023-11-18 [2] CRAN (R 4.3.1) #> R6 2.5.1 2021-08-19 [1] CRAN (R 4.3.0) #> rappdirs 0.3.3 2021-01-31 [1] CRAN (R 4.3.0) #> rcmdcheck 1.4.0 2021-09-27 [2] CRAN (R 4.3.0) #> rematch2 2.1.2 2020-05-01 [1] CRAN (R 4.3.0) #> remotes 2.4.2.1 2023-07-18 [2] CRAN (R 4.3.0) #> reprex 2.1.0 2024-01-11 [2] CRAN (R 4.3.1) #> rex 1.2.1 2021-11-26 [1] CRAN (R 4.3.0) #> rlang 1.1.3 2024-01-10 [1] CRAN (R 4.3.1) #> rmarkdown 2.25 2023-09-18 [1] CRAN (R 4.3.1) #> rprojroot 2.0.4 2023-11-05 [1] CRAN (R 4.3.1) #> RSQLite 2.3.5 2024-01-21 [2] CRAN (R 4.3.1) #> rstudioapi 0.15.0 2023-07-07 [1] CRAN (R 4.3.0) #> sessioninfo 1.2.2 2021-12-06 [2] CRAN (R 4.3.0) #> styler 1.10.2 2023-08-29 [2] CRAN (R 4.3.0) #> tibble 3.2.1 2023-03-20 [1] CRAN (R 4.3.0) #> utf8 1.2.4 2023-10-22 [1] CRAN (R 4.3.1) #> vctrs 0.6.5 2023-12-01 [1] CRAN (R 4.3.1) #> whoami 1.3.0 2019-03-19 [2] CRAN (R 4.3.0) #> withr 3.0.0 2024-01-16 [1] CRAN (R 4.3.1) #> xfun 0.42 2024-02-08 [1] CRAN (R 4.3.1) #> xml2 1.3.6 2023-12-04 [1] CRAN (R 4.3.1) #> xmlparsedata 1.0.5 2021-03-06 [2] CRAN (R 4.3.0) #> xopen 1.0.0 2018-09-17 [2] CRAN (R 4.3.0) #> yaml 2.3.8 2023-12-11 [1] CRAN (R 4.3.1) #> #> [1] /Users/nick/Library/R/arm64/4.3/library #> [2] /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library #> #> ────────────────────────────────────────────────────────────────────────────── ```

Here are some more thoughts of mine on some of the issues {goodpractice} identified.

write unit tests for all functions

There are currently not tests, and I think that the package could benefit from tests to ensure the code behaves as they expect. testing basics provides a good overview

Writing short and simple functions

There is duplicated code in the package, for example, design_cont() and design_ord() reuse a lot of the same code, and a lot of that code has to do with printing output. This output feels like it could benefit from a new print method. I will write more about this in a separate review issue

Using `<-` for assignment instead of `=`

Not import packages as a whole

You do not need to use #' @import stats - you can instead use usethis::use_package("stats") to add the package to imports, then you can refer to the package with the syntax pkg::fun.

fix this R CMD check NOTE: Found the following hidden files and directories: .github

This can be fixed with:

usethis::use_build_ignore(".github")

Link to review: https://github.com/openjournals/joss-reviews/issues/6322

RhysPeploe commented 6 months ago

I didn't know about this package - very handy!

Tpatni719 commented 6 months ago

Thank you for the comments and sharing the name of the package! Regarding the unit tests, I have discussed it with @RhysPeploe. Please see issue #4.
I have fixed the hidden files issue based on your recommendation. And thank you so much for the usethis::use_package("stats") pointer. It is really helpful and I will implement this and the remaining suggestions in my future builds.

njtierney commented 5 months ago

Hi @Tpatni719

Thanks for addressing some of these concerns!

Running goodpractice::gp() I get:

── GP gsMAMS ───────────────────────────────────────────────────────────────────────────

It is good practice to

  ✖ write unit tests for all functions, and all package code in
    general. 71% of code lines are covered by test cases.

    R/logranktest.R:15:NA
    R/logranktest.R:17:NA
    R/logranktest.R:18:NA
    R/logranktest.R:19:NA
    R/logranktest.R:20:NA
    ... and 483 more lines

  ✖ write short and simple functions. These functions have high
    cyclomatic complexity (>50): op_power_ord (91), op_power_cont (86),
    op_fwer_ord (82), op_fwer_cont (77), op_power_surv (73), op_fwer_surv
    (64), size_surv (54). You can make them easier to reason about by
    encapsulating distinct steps of your function into subfunctions.
  ✖ add a "URL" field to DESCRIPTION. It helps users find information
    about your package online. If your package does not have a homepage, add
    an URL to GitHub, or the CRAN package package page.
  ✖ add a "BugReports" field to DESCRIPTION, and point it to a bug
    tracker. Many online code hosting services provide bug trackers for free,
    https://github.com, https://gitlab.com, etc.
  ✖ avoid long code lines, it is bad for readability. Also, many
    people prefer editor windows that are about 80 characters wide. Try make
    your lines shorter than 80 characters

    R/design_cont.R:2:81
    R/design_cont.R:9:81
    R/design_cont.R:11:81
    R/design_cont.R:17:81
    R/design_cont.R:19:81
    ... and 228 more lines

  ✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...), 1:NROW(...) and
    1:NCOL(...) expressions. They are error prone and result 1:0 if the
    expression on the right hand side is zero. Use seq_len() or seq_along()
    instead.

    R/design_cont.R:20:35
    R/design_cont.R:26:36
    R/design_ord.R:26:35
    R/design_ord.R:31:36
    R/design_surv.R:35:35
    ... and 17 more lines

  ✖ not import packages as a whole, as this can cause name clashes
    between the imported packages. Instead, import only the specific functions
    you need.
  ✖ avoid 'T' and 'F', as they are just variables which are set to the
    logicals 'TRUE' and 'FALSE' by default, but are not reserved words and
    hence can be overwritten by the user.  Hence, one should always use 'TRUE'
    and 'FALSE' for the logicals.

    R/op_fwer_ord.R:NA:NA
    R/op_fwer_ord.R:NA:NA
    R/op_fwer_ord.R:NA:NA
    R/op_fwer_ord.R:NA:NA
    R/op_power_ord.R:NA:NA
    ... and 3 more lines

────────────────────────────────────────────────────────────────────────────────────────

If you can address the following I can tick this off from review:

  ✖ add a "URL" field to DESCRIPTION. It helps users find information
    about your package online. If your package does not have a homepage, add
    an URL to GitHub, or the CRAN package package page.
  ✖ add a "BugReports" field to DESCRIPTION, and point it to a bug
    tracker. Many online code hosting services provide bug trackers for free,
    https://github.com, https://gitlab.com, etc.
  ✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...), 1:NROW(...) and
    1:NCOL(...) expressions. They are error prone and result 1:0 if the
    expression on the right hand side is zero. Use seq_len() or seq_along()
    instead.

  ✖ not import packages as a whole, as this can cause name clashes
    between the imported packages. Instead, import only the specific functions
    you need.
  ✖ avoid 'T' and 'F', as they are just variables which are set to the
    logicals 'TRUE' and 'FALSE' by default, but are not reserved words and
    hence can be overwritten by the user.  Hence, one should always use 'TRUE'
    and 'FALSE' for the logicals.

Tpatni719 commented 5 months ago

I have addressed all the issues except the third one because some of the loops are not that straightforward and don't have typical start and stop points.

njtierney commented 5 months ago

When I run goodpractice::gp() I get the following changes that still need to be made:

✖ avoid 1:length(...),
    1:nrow(...), 1:ncol(...),
    1:NROW(...) and 1:NCOL(...)
    expressions. They are error prone
    and result 1:0 if the expression on
    the right hand side is zero. Use
    seq_len() or seq_along() instead.

    R/design_cont.R:19:35
    R/design_cont.R:25:36
    R/design_ord.R:25:35
    R/design_ord.R:30:36
    R/design_surv.R:34:35
    ... and 17 more lines

  ✖ not import packages as a
    whole, as this can cause name
    clashes between the imported
    packages. Instead, import only the
    specific functions you need.

Regarding the first one,

 avoid 1:length(...),
    1:nrow(...), 1:ncol(...),
    1:NROW(...) and 1:NCOL(...)

I do not understand the point:

except the third one because some of the loops are not that straightforward and don't have typical start and stop points.

The code 1:length(x) purely depends on the length of x, and the particular issue that this is protecting against is when there is a bug where x is of zero length, which can happen - although not intentionally.

The issue is that the code 1:length(numeric()) will return 1 0:

x <- numeric()
1:length(x)
#> [1] 1 0

1:seq_along(length(x))
#> [1] 1

^{Created on 2024-04-22 with reprex v2.1.0}

Session info

``` r sessioninfo::session_info() #> ─ Session info ─────────────────────────────────────────────────────────────── #> setting value #> version R version 4.3.3 (2024-02-29) #> os macOS Sonoma 14.3.1 #> system aarch64, darwin20 #> ui X11 #> language (EN) #> collate en_US.UTF-8 #> ctype en_US.UTF-8 #> tz Australia/Hobart #> date 2024-04-22 #> pandoc 3.1.1 @ /Applications/RStudio.app/Contents/Resources/app/quarto/bin/tools/ (via rmarkdown) #> #> ─ Packages ─────────────────────────────────────────────────────────────────── #> package * version date (UTC) lib source #> cli 3.6.2 2023-12-11 [1] CRAN (R 4.3.1) #> digest 0.6.35 2024-03-11 [1] CRAN (R 4.3.1) #> evaluate 0.23 2023-11-01 [1] CRAN (R 4.3.1) #> fastmap 1.1.1 2023-02-24 [1] CRAN (R 4.3.0) #> fs 1.6.3 2023-07-20 [1] CRAN (R 4.3.0) #> glue 1.7.0 2024-01-09 [1] CRAN (R 4.3.1) #> htmltools 0.5.8.1 2024-04-04 [1] CRAN (R 4.3.1) #> knitr 1.45 2023-10-30 [1] CRAN (R 4.3.1) #> lifecycle 1.0.4 2023-11-07 [1] CRAN (R 4.3.1) #> magrittr 2.0.3 2022-03-30 [1] CRAN (R 4.3.0) #> purrr 1.0.2 2023-08-10 [1] CRAN (R 4.3.0) #> R.cache 0.16.0 2022-07-21 [2] CRAN (R 4.3.0) #> R.methodsS3 1.8.2 2022-06-13 [2] CRAN (R 4.3.0) #> R.oo 1.26.0 2024-01-24 [2] CRAN (R 4.3.1) #> R.utils 2.12.3 2023-11-18 [2] CRAN (R 4.3.1) #> reprex 2.1.0 2024-01-11 [2] CRAN (R 4.3.1) #> rlang 1.1.3 2024-01-10 [1] CRAN (R 4.3.1) #> rmarkdown 2.26 2024-03-05 [1] CRAN (R 4.3.1) #> rstudioapi 0.16.0 2024-03-24 [1] CRAN (R 4.3.1) #> sessioninfo 1.2.2 2021-12-06 [2] CRAN (R 4.3.0) #> styler 1.10.3 2024-04-07 [2] CRAN (R 4.3.1) #> vctrs 0.6.5 2023-12-01 [1] CRAN (R 4.3.1) #> withr 3.0.0 2024-01-16 [1] CRAN (R 4.3.1) #> xfun 0.43 2024-03-25 [1] CRAN (R 4.3.1) #> yaml 2.3.8 2023-12-11 [1] CRAN (R 4.3.1) #> #> [1] /Users/nick/Library/R/arm64/4.3/library #> [2] /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library #> #> ────────────────────────────────────────────────────────────────────────────── ```

This code not only appears in your for loops but in other places as well.

Regarding

 ✖ not import packages as a
    whole, as this can cause name
    clashes between the imported
    packages. Instead, import only the
    specific functions you need.

Thanks for making these changes! However you need to run devtools::document() in order to finish that process

If you could please fun goodpractice::gp() and return the output when you respond to this that would be great!

Tpatni123 commented 5 months ago

Done! But you will still get the "not import packages as a whole" message because even though I have used :: operator to reference the functions, I have also used my functions in other functions which is triggering this message.

njtierney commented 5 months ago

If you could please fun goodpractice::gp() and return the output when you respond to this that would be great!

Could you run that and paste the output in the comments? Putting the content inside three backticks like so, will render the code neatly:

[[ paste output of goodpractice::gp() here ]]

Tpatni719 commented 5 months ago

── GP gsMAMS ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

It is good practice to

  ✖ write unit tests for all functions, and all package code in general. 71% of code lines are covered by test cases.

    R/logranktest.R:14:NA
    R/logranktest.R:16:NA
    R/logranktest.R:17:NA
    R/logranktest.R:18:NA
    R/logranktest.R:19:NA
    ... and 483 more lines

  ✖ write short and simple functions. These functions have high cyclomatic complexity (>50): op_power_ord (91), op_power_cont (86),
    op_fwer_ord (82), op_fwer_cont (77), op_power_surv (73), op_fwer_surv (64), size_surv (54). You can make them easier to reason about by
    encapsulating distinct steps of your function into subfunctions.
  ✖ avoid long code lines, it is bad for readability. Also, many people prefer editor windows that are about 80 characters wide. Try
    make your lines shorter than 80 characters

    R\design_cont.R:2:81
    R\design_cont.R:9:81
    R\design_cont.R:11:81
    R\design_cont.R:16:81
    R\design_cont.R:18:81
    ... and 231 more lines

  ✖ not import packages as a whole, as this can cause name clashes between the imported packages. Instead, import only the specific
    functions you need.
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 
>

Tpatni719 / gsMAMS